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

✨ Add OSM Element Tags widget #2499

Merged
merged 3 commits into from
Dec 16, 2024

Conversation

homersimpsons
Copy link
Contributor

@homersimpsons homersimpsons commented Nov 17, 2024

Implements #2453

I based my devlopments on "OSMHistoryWidget", I may not have cleaned up everything yet.

Unresolved questions:

  • How to name this widget? "OSM Data", "OSM Element", "OSM Tags", "OSM Element Tags", other? (converted into comment) => OSM Element Tags
  • Should I "memoize" the retrieved data? => No need
  • Design improvements?
    • I tested to add borders to the table but it didn't look good.
    • I think stripped table would work (I can try to have stripped description list=
    • Maybe use a "raw" display? (just text with "tag:value\ntag2:value" (I do not think it will be easy to read)
    • How to handle long tag value? (I would say overflow ellipsis + tooltip on hover) (I did not implement this, currently ther will be a scrollbar as seen below)
    • Key on top of value? (I tested this and I do not feel like it was easier to read)

Screenshots

(With original name, not updated to "OSM Element Tags" name)

OSM Data loaded: multiple
OSM Data loaded: single
OSM Data loaded: long value
OSM Data loading
OSM Data no element
OSM Data fetch error

@homersimpsons homersimpsons force-pushed the feature/osm-data-widget branch 3 times, most recently from 35f489a to d6d1a11 Compare November 17, 2024 17:35
@homersimpsons homersimpsons marked this pull request as ready for review November 17, 2024 17:38
@homersimpsons homersimpsons force-pushed the feature/osm-data-widget branch 3 times, most recently from c048626 to e4398af Compare November 21, 2024 20:28
@homersimpsons homersimpsons force-pushed the feature/osm-data-widget branch 2 times, most recently from 4cdc531 to d4ed1cf Compare November 21, 2024 21:15
@homersimpsons
Copy link
Contributor Author

@CollinBeczak thanks for the comments. I pushed my final update, I will let you review this. Feel free to comment anything.

@homersimpsons homersimpsons force-pushed the feature/osm-data-widget branch 2 times, most recently from 0cbb694 to eace1cd Compare December 3, 2024 21:28
@homersimpsons homersimpsons changed the title ✨ Add OSM Data widget ✨ Add OSM Element Tags widget Dec 3, 2024
@homersimpsons homersimpsons force-pushed the feature/osm-data-widget branch from eace1cd to c835b26 Compare December 3, 2024 21:33
@CollinBeczak
Copy link
Collaborator

Looks good to me! The design at the moment works and looks good.

@CollinBeczak CollinBeczak self-requested a review December 9, 2024 19:46
@jake-low jake-low merged commit 27e2819 into maproulette:main Dec 16, 2024
@jake-low
Copy link
Contributor

Thanks for contributing this @homersimpsons! It'll be deployed to production in our next release; I'll let you know when that happens.

@jake-low
Copy link
Contributor

This is now available on maproulette.org 🙂

@homersimpsons
Copy link
Contributor Author

homersimpsons commented Dec 23, 2024

This is now available on maproulette.org 🙂

Thank you for noticing me. I tried it out, but I may have made a mistake somewhere because:

  1. The selected feature is not updated on page change (the list of features is correctly updated), hence when switching to the next task it will keep the old element. Do you have any idea where the issue comes from ?
  2. The feature is re-fetched every time the user re-focus the windows, I should add refetchOnWindowFocus: false.

@CollinBeczak
Copy link
Collaborator

This is now available on maproulette.org 🙂

Thank you for noticing me. I tried it out but I may have made a mistake somewhere because it looks like the list of elements is not updated, hence when switching to the next task it will keep the old element. Do you have any idea where the issue comes from ?

The change that resolves the issue you are seeing is in this pr: #2510. I will let you know when we are planning to deploy the change when I find out.

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.

4 participants