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

Feature/1066/metric descriptors in ribbon bar #3273

Merged
merged 46 commits into from
May 27, 2023

Conversation

phanlezz
Copy link
Collaborator

@phanlezz phanlezz commented Apr 20, 2023

Attribute Descriptors / Metric Descriptors for the Ribbon Bar (Metric Choosers) & Metric Links in Sidebar

Closes #1066

Description

This adds a tooltip (in form of a html title tag) to the metric choosers in the ribbon bar.
Metric Chooser entries will get a clear name display in form of a subtitle.
Metric names in the sidebar will be changed to external links that refer to the documentation if such an field exists in the attributeDescriptors.

Screenshots

grafik
grafik

@phanlezz phanlezz self-assigned this Apr 20, 2023
@phanlezz
Copy link
Collaborator Author

I fixed a bug where the max metric values would be displayed for the selected metrics.
Do we want to display them in the metric choosers?
image

@shaman-apprentice
Copy link
Contributor

shaman-apprentice commented May 22, 2023

Hey @shaman-apprentice, while the rerendering worked for the metricChooser tests, I can't it to work for the other choosers (namely area-, height-, color-, edgeMetricChooser). Any idea how to fix the tests?

When I check getLastAction for the store it returns the correct result (set..Metric), but the metric inside the store doesn't seem to get updated, as it always stays at the value set while configuring.

I was so free to push the needed adjustment for the area metric chooser with this commit.

The mocked store is really stupid (which is nice, as it separates concerns and is performant). It is just a mocked value and a stream of dispatched actions. For UI changes we need to update the mocked value manually. Or use the real store instead of the mocked store. Also see there official testing guide https://v8.ngrx.io/guide/store/testing ;)

I guess it previously worked as the selected value was just the displayed value.

@phanlezz phanlezz marked this pull request as ready for review May 24, 2023 13:48
@phanlezz
Copy link
Collaborator Author

@shaman-apprentice I noticed the e2e-tests do not perform very well when run in parallel (compared to main). They are unstable and take forever.
Any idea how this can be improved?

@ce-bo
Copy link
Collaborator

ce-bo commented May 26, 2023

Could you please adjust the following things?

  • Remove the textual Metric Link from the html title tag in the sidebar (secondary and primary metrics) as the user wouldn't be able to click on it within the tooltip when it pops up. It is sufficient that the metrics are linked and the user can click on them.
  • Please add the hint and low values to the metric tooltips in the legend
  • Please add the link to the metric documentation for metrics in the legend as an external link icon like it is done in the sidebar.

Besides that I would like to discuss with you, if it wouldn't make more sense to show the short description of a metric instead of the metric title within the metric choosers. In the following screenshot you will notice that the metric name and the description below is more or less the same:
image

I found one issue when a metric is selected. The metric name is duplicated by showing the metric key and title.
image

@phanlezz
Copy link
Collaborator Author

* Remove the textual Metric Link from the html title tag in the sidebar (secondary and primary metrics) as the user wouldn't be able to click on it within the tooltip when it pops up. It is sufficient that the metrics are linked and the user can click on them.

I included the link to the tooltip, because usually it is very important for an user to see where a hyperlink leads to (as we do when a selected node has a link in the sidebar header).
I will remove it again, and the browser bottom left hyperlink preview will give this information.
Remove in the sidebar and legend and keep it in metric choosers ? I need to adjust or add a second pipe in that case, because at the moment the implementation for all tooltips is the same.

* Please add the hint and low values to the metric tooltips in the legend

They are already included. Only the fallback titles do not have a tooltip, because all information is already displayed.
Please make sure to do a fresh build, this might be a bug otherwise.

Fallback (default, no tooltip):
grafik
If descriptors are available.
grafik

* Please add the link to the metric documentation for metrics in the legend as an external link icon like it is done in the sidebar.

👍

Besides that I would like to discuss with you, if it wouldn't make more sense to show the short description of a metric instead of the metric title within the metric choosers. In the following screenshot you will notice that the metric name and the description below is more or less the same: image

Here is a preview of what it would look like. (UWQHD)
grafik
And a comparison with sub 1080p
grafik

I found one issue when a metric is selected. The metric name is duplicated by showing the metric key and title. image

Can you give me some more details, because this might be a bug. I removed the (maximum value) from the metric choosers and in the same action the subtitle. Make sure to run npm ci and maybe delete any cache/build folders. As you can see in the screenshots I provided, that there is no additional text in the metric choosers. The tests for the metric choosers expect, that there is no additional number/text, so if this is a bug, the tests need to be reworked too.

@ce-bo
Copy link
Collaborator

ce-bo commented May 26, 2023

Remove in the sidebar and legend and keep it in metric choosers ? I need to adjust or add a second pipe in that case, because at the moment the implementation for all tooltips is the same.

That's fine

Please make sure to do a fresh build, this might be a bug otherwise.

I checked out the branch this morning and did an npm i and npm run dev. I used the map you have provided to me a few weeks ago. I am wondering why the hints are shown in the sidebar but not in the legend for the same metric.

Here is a preview of what it would look like. (UWQHD)

Thank you! Let's discuss this in person.

Can you give me some more details, because this might be a bug. I removed the (maximum value) from the metric choosers and in the same action the subtitle. Make sure to run npm ci and maybe delete any cache/build folders. As you can see in the screenshots I provided, that there is no additional text in the metric choosers. The tests for the metric choosers expect, that there is no additional number/text, so if this is a bug, the tests need to be reworked too.

When I load the map you have provided to me, the text in the metric choosers is looking like this. We can have a look together but it might be a bug.

@ce-bo
Copy link
Collaborator

ce-bo commented May 26, 2023

My bad. The bugs are gone after rebuilding/pulling the current state. Sorry.

@phanlezz
Copy link
Collaborator Author

[...] I am wondering why the hints are shown in the sidebar but not in the legend for the same metric.

(AD = attributeDescriptor)
Well my thought process was like this:
In the sidebar it is the key that is displayed and all AD or the fallback title as tooltip.
In the legend it is always metric: title (key) that is displayed (if fallback available) and all AD displayed as tooltip if present.

So in the legend all information is already displayed, and there is nothing more that could be a tooltip, which is different from the sidebar, because we only show the metric key there and can add the title to the tooltip.

Copy link
Contributor

@shaman-apprentice shaman-apprentice left a comment

Choose a reason for hiding this comment

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

Early approve: I only read the code and did no testing. Cool feature and nice work! 👍

@shaman-apprentice
Copy link
Contributor

shaman-apprentice commented May 26, 2023

@shaman-apprentice I noticed the e2e-tests do not perform very well when run in parallel (compared to main). They are unstable and take forever. Any idea how this can be improved?

When a test fails, there will be a lot of timeout hits before failing. That is why they take forever if failing.

When they are stable if executed in serial I wouldn't spent too much time into it, but invest the time into the ongoing migration to Playwright (JFI: @Hall-Ma @ce-bo )

@sonarcloud
Copy link

sonarcloud bot commented May 27, 2023

[CodeCharta Analysis] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@phanlezz phanlezz enabled auto-merge (squash) May 27, 2023 11:11
@sonarcloud
Copy link

sonarcloud bot commented May 27, 2023

[CodeCharta Visualization] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

94.2% 94.2% Coverage
0.0% 0.0% Duplication

@phanlezz phanlezz merged commit 457ddcf into main May 27, 2023
@phanlezz phanlezz deleted the feature/1066/metric-descriptors-in-ribbon-bar branch May 27, 2023 11:13
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.

Provide tooltip / description for each metric
4 participants