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

Implement editing of domain bounds #557

Merged
merged 3 commits into from
Mar 11, 2021
Merged

Implement editing of domain bounds #557

merged 3 commits into from
Mar 11, 2021

Conversation

axelboc
Copy link
Contributor

@axelboc axelboc commented Mar 11, 2021

Some screenshots for 😮, 😋 and 😍 ... (at least! 😏)

image

image

image

image

image

@axelboc axelboc requested a review from loichuder March 11, 2021 09:48
@axelboc axelboc mentioned this pull request Mar 11, 2021
23 tasks
Copy link
Member

@loichuder loichuder left a comment

Choose a reason for hiding this comment

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

I like the design very much ! Looking clean ! 👏

A few nitpicks here and there

src/h5web/vis-packs/core/heatmap/utils.ts Outdated Show resolved Hide resolved
src/h5web/toolbar/controls/DomainSlider/DomainSlider.tsx Outdated Show resolved Hide resolved
src/h5web/toolbar/controls/DomainSlider/DomainSlider.tsx Outdated Show resolved Hide resolved
src/h5web/toolbar/controls/DomainSlider/BoundEditor.tsx Outdated Show resolved Hide resolved
src/h5web/toolbar/controls/DomainSlider/BoundEditor.tsx Outdated Show resolved Hide resolved
@axelboc axelboc force-pushed the edit-domain branch 2 times, most recently from bfd05f3 to de2a06d Compare March 11, 2021 11:44
@axelboc axelboc requested a review from loichuder March 11, 2021 11:44
Copy link
Member

@loichuder loichuder left a comment

Choose a reason for hiding this comment

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

Great !

@loichuder
Copy link
Member

Before you merge, I noticed a small issue:

  • Click on the Tune button to toggle edit
  • Click on it again to toggle edit off

The tooltip disappears and do not appear again, even when moving thumbs. One must move the pointer away from the slider to make it reappear.

@axelboc
Copy link
Contributor Author

axelboc commented Mar 11, 2021

The tooltip disappears and do not appear again, even when moving thumbs. One must move the pointer away from the slider to make it reappear.

Yeah, it's on purpose. You would only click this button to explicitly close the popup and cancel your edits. If you clicked and the popup stayed, it'd just feels confusing... wouldn't it? At least I thought it was when I first tried it, but that was before I had the input fields...

@axelboc
Copy link
Contributor Author

axelboc commented Mar 11, 2021

Neh you're right, now with the input fields, the behaviour of keeping the tooltip open makes more sense.

@axelboc axelboc force-pushed the edit-domain branch 2 times, most recently from 71debf7 to 7f7da3e Compare March 11, 2021 14:13
@axelboc
Copy link
Contributor Author

axelboc commented Mar 11, 2021

/approve

@axelboc axelboc merged commit 9e6b205 into main Mar 11, 2021
@axelboc axelboc deleted the edit-domain branch March 11, 2021 14:26
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