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

Introduce intent-based colors, remove colors.ts #493

Open
wants to merge 16 commits into
base: feature/a11ymap
Choose a base branch
from

Conversation

opyh
Copy link
Member

@opyh opyh commented Feb 11, 2025

  • Removes hard-coded CSS color values where possible and points to the according Radix variables (fixing dark/bright mode)
  • introduces reusable CSS color aliases that you can use to signalize an intent in the UI (danger, warning, success)
  • tries to use CSS color variables instead of JS-based color where possible. This means, for example, that map marker colors are derived from the CSS.

Further reading:

On the way I've dared a mini refactoring to fix the totally-broken looking mapping events list UI. I hope that's okay :)

IMO the UI changes are only subtle and we'll have another round of polishing, so from my perspective, reviewing just the code would be enough here.

📎 Related Ticket

This code is based on #490. It needs rebasing onto feature/a11ymap before merging.

💪 I have tested my code - [x] The feature deployment works. - [x] The automated tests are passing. - [x] I have manually tested this feature - [x] in Chrome - [ ] in Firefox - [ ] in Safari
🧼 I have cleaned up my code - [x] I have removed dependencies that were just for testing. - [x] I have removed debug logging. - [x] My code does not generate new warnings. - [x] My code does not depend on new vulnerable packages. - [x] The commit messages are precise and make sense (rebase the PR with `--interactive` if applicable, keeping commits in sensible chunks if possible).
🔍 I have performed a self-review of my code - [x] My code is self-documenting or has links to necessary documentation. - [x] New function and variables names can be understood by new developers with basic project knowledge. - [x] The feature fits the design.

@opyh opyh marked this pull request as ready for review February 11, 2025 20:37
@opyh opyh requested review from kriskbx and Mayaryin February 12, 2025 14:36
Base automatically changed from chore/merge-editing-and-linting to feature/a11ymap February 12, 2025 14:43
@opyh opyh force-pushed the chore/refactor-shared-colors branch from bca44d4 to 14a0dd4 Compare February 12, 2025 14:49
@kriskbx kriskbx self-assigned this Feb 17, 2025
Copy link
Member

@kriskbx kriskbx left a comment

Choose a reason for hiding this comment

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

Hey, thanks a ton for your amazing PR ❤️

Great work on the mapping event UI! 💪

issue (non-blocking): There is still an old loading indicator in use that is visually broken now due to the changes. I think this is the src/components/Map/LoadingIndicator. When we're at it, we could replace it with the new one. Not blocking, though.
image

issue: When I try to open a mapping event locally and on the feature branch deployment, I get this error:
image
Don't know if this is related to the recent changes, but I cannot test the rest of the mapping event UI 🙁

Copy link
Member

Choose a reason for hiding this comment

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

note (non-blocking): I think we should have communicated better. All these search and toolbar related files have been deleted and/or replaced in the need-picker branch, so your changes here will most probably either be discarded or conflict heavily.

Copy link
Member Author

@opyh opyh Feb 18, 2025

Choose a reason for hiding this comment

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

I've expected this, no biggie :) I can merge / rebase.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can merge this after the need picker branch.

*/

:root {
--rating-yes: var(--grass-9);
Copy link
Member

@kriskbx kriskbx Feb 17, 2025

Choose a reason for hiding this comment

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

issue/discussion: I think this is a fantastic time to talk about ubiquitous language and define a common term for accessibility rating together, that we will use throughout the codebase (in the future), but also in our own communication.

Rating is a quite general term and on its own could be misunderstood easily. So rating-yes doesn't make sense on it's own. accessibility-rating-yes is better and more clear, but what does "yes" in this context mean? Rating implies (at least for me) that there's a numeric scale, which there isn't. I dislike "yes", "no", "limited" as it is not really self-explanatory. I also like "partial" more than "limited". Partial means to me that some things are accessible and some things are not. But what does limited mean regarding accessibility? It's limited to what degree? What kind of abilities do I need to use it? We already use accessibilityGrade in the codebase, which I think is also not great.

I have a few ideas in mind: Rating per se is not bad, so if we want to stick with rating, using it as an adjective in combination with accessibility could make it clearer: --rated-fully-accessible, --rated-partially-accessible, --unknown-accessibility-rating, --rated-not-accessible. The thing is, "unknown" doesn't really fit here.

If we use it as noun, "not-accessible" doesn't really fit: --accessibility-rating-full, --accessibility-rating-partial, --accessibility-rating-unknown, --accessibility-rating-not-accessible (--accessibility-rating-not is not self-explanatory).

Alternatively, we could also disconnect the rating from accessibility itself and create some kind of scale: --rating-good, --rating-mediocre, --rating-bad, --rating-unknown.

Do you have any ideas/opinions? I'm eager to hear all of those 🙂

Copy link
Member Author

@opyh opyh Feb 18, 2025

Choose a reason for hiding this comment

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

Funny, I switched to exactly the naming you proposed in the first part of your comment, and ran into the same consistency issue.

Ideally, there would be a data model that has something like this: https://sozialhelden.github.io/a11yjson/describing-objects/type-aliases/#wheelchairaccessibilitygrade

…but we don't use such a concept yet, and we won't get rid of OSM's yes/no/limited values anytime soon.

I really like --rating-good, --rating-mediocre, --rating-bad, --rating-unknown. If you and @Mayaryin would agree I'd do a small refactoring to get us there, and I'd add it into this PR. What do you think?

@kriskbx kriskbx assigned opyh and unassigned kriskbx Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants