Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some design fixes #252

Merged
merged 8 commits into from
Aug 15, 2024
Merged

Some design fixes #252

merged 8 commits into from
Aug 15, 2024

Conversation

Lionqueen94
Copy link
Contributor

@Lionqueen94 Lionqueen94 commented Aug 12, 2024

Fixes (from #101):

  • Add hover state in navigation menu on /1/input/33/... (see Figma)

  • Check colors of icons (current page, empty input) in the nav menu. Have been updated in Figma, they were a subtly different color then the texts, is now consistent with text

  • font-weights: het is ok om de 500 en 600 te vervangen door 700 (gebeurt op een aantal buttons al, daar wordt een bold over de 500 weight gezet, waardoor die 500 al niks meer doet). Nb: alleen voor de DM Sans teksten. De 500 op Space Grotesk (de badges) moet zo blijven

and removes the right padding on the polling station choice form as requested on Figma

jorisleker
jorisleker previously approved these changes Aug 12, 2024
@jorisleker
Copy link
Contributor

jorisleker commented Aug 12, 2024

Te snel op approve gedrukt:

Er gaat nog iets 'mis' met de hover state. Het label is namelijk groter dan de a. Het lijkt alsof het hele label klikbaar is (en idealiter is dat ook zo), terwijl alleen de a nu klikbaar is. Zie screenshot hieronder. Ik heb de a voor het gemak even een background gegeven.

Je merkt het het snelst/makkelijkst als je op het stukje probeert te klikken waar het pijltje staat, maar ook de padding rondom de a is niet klikbaar (terwijl cursor daar wel een pointer is)
Scherm_afbeelding 2024-08-12 om 11 31 28

@Lionqueen94 Lionqueen94 marked this pull request as draft August 12, 2024 10:48
@Lionqueen94
Copy link
Contributor Author

@jorisleker Please review again, I fixed the issue you commented

@jorisleker
Copy link
Contributor

Sorry, I should have been a bit more clear.

the clickable area should be just as big as the area we change in the hover state. You now 'fixed' it by removing the pointer cursor. The fix I meant to request was to extend the clickable area to be as big as the area affected by the hover state. Bigger click-targets is better. I know this is probably more work because you need to change both html and css instead of just css.

@Lionqueen94
Copy link
Contributor Author

Sorry, I should have been a bit more clear.

the clickable area should be just as big as the area we change in the hover state. You now 'fixed' it by removing the pointer cursor. The fix I meant to request was to extend the clickable area to be as big as the area affected by the hover state. Bigger click-targets is better. I know this is probably more work because you need to change both html and css instead of just css.

This is what you desire I think, I did make sure the icon has a z-index of 1 so that when you hover it you can see the alt text. The whole hover state section is now clickable, except for the icon.

jorisleker
jorisleker previously approved these changes Aug 12, 2024
Copy link
Contributor

@praseodym praseodym left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this breaks the icon in the navigation column in Safari:

main branch:
Screenshot 2024-08-13 at 11 35 52

This branch:
Screenshot 2024-08-13 at 11 35 56

@Lionqueen94 Lionqueen94 requested a review from jorisleker August 13, 2024 09:59
@Lionqueen94 Lionqueen94 requested a review from lkleuver August 13, 2024 10:17
@jorisleker jorisleker added this pull request to the merge queue Aug 15, 2024
Merged via the queue into main with commit e200f39 Aug 15, 2024
7 checks passed
@jorisleker jorisleker deleted the some-design-fixes branch August 15, 2024 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants