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

FeaturePanel: Highlight colours #591

Merged
merged 3 commits into from
Sep 24, 2024
Merged

FeaturePanel: Highlight colours #591

merged 3 commits into from
Sep 24, 2024

Conversation

Dlurak
Copy link
Collaborator

@Dlurak Dlurak commented Sep 23, 2024

Example: https://osmapp.org/relation/14995860

Description

Highlight colors by providing a background color for the colour tag.

image
image

Design choices

  • I'm really unsure about showing the hex code after a word.
  • Is it better to use a background color like in this pr or just a small colored space like osm.org?
  • I think it is preferred to show the values as they are in osm. But if you want it would be easy to convert the hex codes to human readable color names as I have developed a library for that.

Copy link

vercel bot commented Sep 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
osmapp ✅ Ready (Inspect) Visit Preview Sep 24, 2024 2:38pm

Copy link

sentry-io bot commented Sep 23, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: src/components/FeaturePanel/Properties/renderValue.tsx

Function Unhandled Issue
getHumanValue TypeError: Cannot read properties of undefined (reading 'replace') ...
Event Count: 10 Affected Users: 1
getHumanValue TypeError: undefined is not an object (evaluating 't.replace') ...
Event Count: 3 Affected Users: 1
📄 File: src/components/SearchBox/options/openstreetmap.tsx (Click to Expand)
Function Unhandled Issue
getOsmOptions TypeError: a is undefined /[[...all]]
Event Count: 7 Affected Users: 2
getOsmOptions TypeError: undefined is not an object (evaluating 'a.match') ...
Event Count: 2 Affected Users: 1
---

Did you find this useful? React with a 👍 or 👎

zbycz
zbycz previously approved these changes Sep 24, 2024
Copy link
Owner

@zbycz zbycz left a comment

Choose a reason for hiding this comment

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

I'm really unsure about showing the hex code after a word.

I would suggest leaving only the human readable color label and not adding hex.

Is it better to use a background color like in this pr or just a small colored space like osm.org?

Both is fine, the box like on osm.org is maybe better, because you don't have to change text color to make it readable + it doesnt' burn too much people's eyes eg in dark mode 🙂

I think it is preferred to show the values as they are in osm. But if you want it would be easy to convert the hex codes to human readable color names as I have developed a library for that.

I think it is most useful to show the value stored in OSM. I don't think it is worth it to transform hex to human readable label (+all the translations). The color is visible near and everyone with right sight would see it and know the name of the color 🙂

And last but not least, the color tag is not one of the most important in OSM. I prefer having as little code as possible to make it usable, but not too smart. (As always I am open to discussion)

@Dlurak Dlurak merged commit 938f85e into zbycz:master Sep 24, 2024
2 checks passed
Copy link

sentry-io bot commented Sep 25, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ TypeError: t is undefined /[[...all]] View Issue
  • ‼️ TypeError: t.match is not a function /[[...all]] View Issue

Did you find this useful? React with a 👍 or 👎

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