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

feat measurement visibility #2773

Closed
wants to merge 4 commits into from

Conversation

ladeirarodolfo
Copy link
Collaborator

@ladeirarodolfo ladeirarodolfo commented Apr 6, 2022

Includes

  • Visibility icon at measurement panel
  • Visibility functionality
  • Hotkeys assigned for toggling visibility: m for current active measurement, shift+m for all measurements

Screen Shot 2022-04-12 at 17 10 24

PR Checklist

  • Brief description of changes
  • Links to any relevant issues
  • Required status checks are passing
  • User cases if changes impact the user's experience
  • @mention a maintainer to request a review

Make the themeable overlays use a straight configuration value
to demonstrate how config point is just an aid to defining and
optimizing configuration points.
@codecov
Copy link

codecov bot commented Apr 6, 2022

Codecov Report

Merging #2773 (8ee5200) into feat/v3-finding-sites (1a0bd2a) will decrease coverage by 0.13%.
The diff coverage is 0.00%.

❗ Current head 8ee5200 differs from pull request most recent head c125031. Consider uploading reports for the commit c125031 to get more accurate results

Impacted file tree graph

@@                    Coverage Diff                    @@
##           feat/v3-finding-sites    #2773      +/-   ##
=========================================================
- Coverage                  25.26%   25.13%   -0.14%     
=========================================================
  Files                        119      119              
  Lines                       2858     2873      +15     
  Branches                     560      564       +4     
=========================================================
  Hits                         722      722              
- Misses                      1845     1856      +11     
- Partials                     291      295       +4     
Impacted Files Coverage Δ
.../services/MeasurementService/MeasurementService.js 45.50% <0.00%> (-3.69%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a0bd2a...c125031. Read the comment docs.

@cypress
Copy link

cypress bot commented Apr 6, 2022



Test summary

26 0 0 0


Run details

Project Viewers
Status Passed
Commit c125031
Started Apr 14, 2022 7:49 PM
Ended Apr 14, 2022 7:50 PM
Duration 01:17 💡
OS Linux Debian - 10.6
Browser Chrome 86

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@wayfarer3130
Copy link
Contributor

Generally the changes are mostly ok. A couple of things that I think would make it a bit more useful:

  1. Remove the icon from the table to toggle visibility. Having too many icons will be a problem. If we can figure out how to make right-click in the table bring up the context menu, then that would be useful.
  2. If the user clicks the measurement for any reason, make it visible
  3. Have a global hide-measurements command, acting on all objects (similarly a global show all)

I think the user experience is better that way. We can talk tomorrow about how it works.

@ladeirarodolfo
Copy link
Collaborator Author

Generally the changes are mostly ok. A couple of things that I think would make it a bit more useful:

  1. Remove the icon from the table to toggle visibility. Having too many icons will be a problem. If we can figure out how to make right-click in the table bring up the context menu, then that would be useful.
  2. If the user clicks the measurement for any reason, make it visible
  3. Have a global hide-measurements command, acting on all objects (similarly a global show all)

I think the user experience is better that way. We can talk tomorrow about how it works.

  1. We definitely can make it, shall we? I dont disagree but as a user I would prefer to have the icon already displayed instead of having to do two actions to hide it (one to open menu other to hide/show)
  2. Measurement you mean on viewport? or on the table? If first one is the case: This is really doable, but, the solution should go under image library, otherwise it will be a workaround. Shall we? I can not estimate the effort yet. What about second one: what should happen if user clicks on measurement item (on table)?
  3. Alright, and will there be a icon for that? Where to place it.

@ladeirarodolfo
Copy link
Collaborator Author

@wayfarer3130 code is updated with what we discussed internally. Can you take a look please?

refact code review: visibility all, hotkeys

refact code review: revert formatting changes :(

refact code review. remove unecessary files

refact code review: revert formatting changes :(. Minor jsdocs entry
@ladeirarodolfo ladeirarodolfo force-pushed the feat/v3-finding-site-visibility branch from 8ee5200 to c125031 Compare April 14, 2022 19:43
@sedghi
Copy link
Member

sedghi commented Jan 24, 2023

Do you need this? If so can you please rebase on v3-stable?

@sedghi sedghi changed the base branch from feat/v3-finding-sites to master June 19, 2023 13:36
@sedghi
Copy link
Member

sedghi commented Jun 19, 2023

what is the status on this? do we still want it? @ladeirarodolfo Do you have plan to work on this?

@sedghi
Copy link
Member

sedghi commented May 29, 2024

will revisit this here
#4095

@sedghi sedghi closed this May 29, 2024
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.

None yet

3 participants