Conversation
✅ Deploy Preview for criipto-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
nmoskaleva
force-pushed
the
update-homepage
branch
from
d76165d to
175e4d8
Compare
nmoskaleva
force-pushed
the
update-homepage
branch
from
175e4d8 to
5da1150
Compare
nmoskaleva
force-pushed
the
update-homepage
branch
from
5da1150 to
6eace93
Compare
jlndk
left a comment
jlndk
left a comment
There was a problem hiding this comment.
Looks incredible!!!!
Just a few small comments but otherwise it LGTM
src/Homepage/eIDCards.tsx
Outdated
| href={`/verify/e-ids/${eid.href}`} | ||
| logoSrc={eid.src} | ||
| logoWidth={eid.width} | ||
| logoHeight={eid.height} |
There was a problem hiding this comment.
Instead of setting an explicit width / height for each img you could consider adding a container with a fixed size and let the logo fill that instead?
That was way we don't have to define an explicit size for every img
There was a problem hiding this comment.
@Trinurt made logos in different sizes so they look good all together... I don't think container will work in this case?
There was a problem hiding this comment.
@Trinurt made logos in different sizes so they look good all together...
@nmoskaleva Consider a more sustainable solution going forward, having to involve an additional person each time we add a logo is not ideal.
Also if the logo were made with specific sizes why does it need to be defined? The image it self would already have the size.
There was a problem hiding this comment.
Right, I might have exaggerated. There is a default size, though :)
There was a problem hiding this comment.
@jlndk The logic behind the tech logos is the same as for the eID logos. We want them to appear the same size when used side by side.
Therefore the logo size is fixed AND (@nmoskaleva ) they are placed in a container with a fixed height (matching the max allowed logo height) and flex width. The logos are top/bottom centered in the container.
This image is from the design system:
There was a problem hiding this comment.
@nmoskaleva The correct fixed height of the logo container is 44 px. Please note that all the square solid logos are 38x38 px - also Kotlin. (Currently you have them at 40x40 px).
Forgot to mention I think the page looks really good and addresses the top concerns we have identified for the target group spot on 🙌
There was a problem hiding this comment.
PS. @nmoskaleva If it is a help I can make a SVG version of the tech logos where they are already in a container with a fixed height? Let me know if you would like me to make that
| import MitIDErhvervLogo from './logos/eids/mitid-erhverv.svg'; | ||
| import PersonalausweisLogo from './logos/eids/personalausweis.svg'; | ||
|
|
||
| const eids = [ |
There was a problem hiding this comment.
Would be preferable if one could NOT forget to add this when adding a page to Verify
There was a problem hiding this comment.
Right. Are you suggesting there is a way to automate it? @mickhansen
There was a problem hiding this comment.
Not necessarily automate but at least type guard. We can explore it in the future, not a blocker for this PR.
| import React, { ReactElement } from 'react'; | ||
| import { LogoCard } from '../components/LogoCard/LogoCard'; | ||
|
|
||
| import Auth0Logo from './logos/integrations/auth0.svg'; |
There was a problem hiding this comment.
File and component names look like they should be Verify specific.
|
Homepage should probably have a TOC when it has contents below the fold with multiple headlines |
|
I have to run now, but will address the rest of the comments by EOD 😬 |
Will be used on the updated homepage for improved navigation and UX.
nmoskaleva
force-pushed
the
update-homepage
branch
2 times, most recently
from
4d53d3b to
f9c81c5
Compare
Great point about below-the-fold content. But I'm worried a ToC might just elongate the page and push the content even further down. Also, it might be worth discussing with UX. Could we revisit this when I'm back? |
@nmoskaleva We have a generic TOC component already, in the right hand side navigation. |
Will be used on the updated homepage to display a list of integrations and eIDs for improved navigation and UX.
mickhansen
force-pushed
the
update-homepage
branch
2 times, most recently
from
92e7177 to
5065d2a
Compare
mickhansen
left a comment
mickhansen
left a comment
There was a problem hiding this comment.
Given what a vast improvement this is, we will merge as is and address the rest of the comments/ideas when Natalia is back
Replace existing generic description of Idura Verify and Idura Signatures with links to integration guides, eIDs and products the user is likely to look for when they're just getting started.
mickhansen
force-pushed
the
update-homepage
branch
from
5065d2a to
eea1eef
Compare
4 participants
Replace generic description of Idura Verify and Idura Signatures with links to integration guides, eIDs and products the user is likely to look for when they're just getting started.
QA: https://deploy-preview-298--criipto-docs.netlify.app/