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

PB-66: Add new babs icons to mapviewer #1058

Merged
merged 15 commits into from
Sep 12, 2024

Conversation

LukasJoss
Copy link
Contributor

@LukasJoss LukasJoss commented Sep 5, 2024

Copy link

cypress bot commented Sep 5, 2024

web-mapviewer    Run #3296

Run Properties:  status check passed Passed #3296  •  git commit a8895f46a7: PB-66: Use class instead of id for tippy in v-for
Project web-mapviewer
Branch Review feat-PB-66-add-new-babs-icons-to-mapviewer
Run status status check passed Passed #3296
Run duration 05m 03s
Commit git commit a8895f46a7: PB-66: Use class instead of id for tippy in v-for
Committer Lukas Joss
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 21
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 210
View all changes introduced in this branch ↗︎

@LukasJoss LukasJoss force-pushed the feat-PB-66-add-new-babs-icons-to-mapviewer branch from 27b5892 to 3b81fc4 Compare September 5, 2024 11:48
@LukasJoss LukasJoss force-pushed the feat-PB-66-add-new-babs-icons-to-mapviewer branch from 2639661 to 0d6c2b8 Compare September 5, 2024 14:22
@LukasJoss LukasJoss force-pushed the feat-PB-66-add-new-babs-icons-to-mapviewer branch 3 times, most recently from cbfede4 to e8c864e Compare September 6, 2024 15:06
Copy link
Contributor

@pakb pakb left a comment

Choose a reason for hiding this comment

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

couple questions about the whole thing :

  • why have this description URL? We won't be using it, as i18n keys are then also given in the list of icons endpoint.
  • I'm wondering if we should "tag" the sets with a lang ISO code value, so that we may have a lang selector alongside the choice of "Civil icons", instead of having 3 times the civil icons in the list
  • When showing description on icons that aren't available in the current app lang, I would show them all (prefixed with ISO code)
  • I would maybe add the description of the icon to the description of the marker, would make sense to know what kind of icon it is also outside of the drawing module

@LukasJoss
Copy link
Contributor Author

couple questions about the whole thing :

* why have this description URL? We won't be using it, as i18n keys are then also given in the list of icons endpoint.

Should the backend for the set instead have a boolean, e.g. has_description?

* I'm wondering if we should "tag" the sets with a `lang` ISO code value, so that we may have a lang selector alongside the choice of "Civil icons", instead of having 3 times the civil icons in the list

I assume this should also be provided by the back-end?

* When showing description on icons that aren't available in the current app lang, I would show them all (prefixed with ISO code)

What do you mean with "icons that aren't available in the current app lang"? All icons should be available in all languages

* I would maybe add the description of the icon to the description of the marker, would make sense to know what kind of icon it is also outside of the drawing module

Copy link
Contributor

@ltkum ltkum left a comment

Choose a reason for hiding this comment

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

Solid work. I'm not sure about the need for the translation parameter in this case (but I did not work with the icons, so maybe some descriptions will have problematic 'keys'), and I would alter the switch a bit, but I think this is otherwise good.

translate
? i18n.t(tp.reference.attributes['data-tippy-content'].value)
: tp.reference.attributes['data-tippy-content'].value
)
Copy link
Contributor

Choose a reason for hiding this comment

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

When you give a key that doesn't exist, i18n will return the key itself and won't translate anything. Unless there is a use case I don't see, I think you don't need to add this 'translate' parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I don't set it to false it will search through the entire dictionary for every icon which makes it laggy

@LukasJoss LukasJoss force-pushed the feat-PB-66-add-new-babs-icons-to-mapviewer branch from 8f337df to 23f9e0b Compare September 10, 2024 09:43
@LukasJoss LukasJoss force-pushed the feat-PB-66-add-new-babs-icons-to-mapviewer branch from edc0082 to 30cc524 Compare September 11, 2024 13:52
@LukasJoss LukasJoss requested a review from pakb September 11, 2024 13:56
@LukasJoss LukasJoss requested review from ltkum and removed request for hansmannj September 11, 2024 13:56
@LukasJoss LukasJoss assigned hansmannj and unassigned hansmannj Sep 11, 2024
@LukasJoss LukasJoss requested a review from hansmannj September 11, 2024 13:57
@hansmannj
Copy link
Member

Not sure if that is expected behavior: The mouseover text for single icons only appears, if the icon grid is expanded.
When only the first row is slightly visible, hovering over the icons does not fire the tooltip with the translations.

@LukasJoss
Copy link
Contributor Author

Not sure if that is expected behavior: The mouseover text for single icons only appears, if the icon grid is expanded. When only the first row is slightly visible, hovering over the icons does not fire the tooltip with the translations.

@hansmannj this is intentional

@LukasJoss LukasJoss requested a review from pakb September 12, 2024 07:27
@LukasJoss LukasJoss merged commit 045cfe1 into develop Sep 12, 2024
6 checks passed
@LukasJoss LukasJoss deleted the feat-PB-66-add-new-babs-icons-to-mapviewer branch September 12, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants