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

Introduce a new blueprint archetype for AxisY configuration in a plot. #5028

Merged
merged 28 commits into from
Feb 7, 2024

Conversation

jleibs
Copy link
Member

@jleibs jleibs commented Feb 2, 2024

What

Part of

Introduce a new archetype for AxisY configuration in a plot.

This contains 2 new components:

  • Range: defines a min / max value for the y-axis of the plot.
  • LockRangeDuringZoom: a boolean indicating whether the range should be locked during zoom

In "LockRange" mode, the Y-axis of the plot will always be locked to the defined range, regardless of whether the range is Auto or a specified range.

In the default mode, the user can zoom arbitrarily in a way that preserves the aspect ratio of the data. When the user resets the view, it will return to the defined range.

image

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@jleibs jleibs added 🟦 blueprint The data that defines our UI include in changelog labels Feb 2, 2024
@jleibs jleibs changed the title Introduce a new archetype for AxisY configuration in a plot. Introduce a new blueprint archetype for AxisY configuration in a plot. Feb 2, 2024
@jleibs jleibs marked this pull request as ready for review February 2, 2024 23:39
@Wumpf
Copy link
Member

Wumpf commented Feb 3, 2024

Doesn't play nice with the manual single-axis zoom on scroll + ctrl + alt: It jumps into a previous zoom level and goes from there but doesn't change the blueprint setting as expected

@jleibs
Copy link
Member Author

jleibs commented Feb 3, 2024

Ahh, I should use the active range when doing that single axis zoom. But, I don't think I would expect it to change the blueprint as a result.

Base automatically changed from jleibs/mixed_scope to main February 4, 2024 16:29
@Wumpf Wumpf self-requested a review February 4, 2024 16:30
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

interesting compromise, easier to understand for sure. There's one small remaining issue but we can take that later:

  • set a y range
  • don't lock zoom
  • move the plot beyond the camera
  • do ctrl+alt zooming: It now jumps to the y previously range in manner it should only do on double click

not a biggy, I doubt lot of people are discovering ctrl+alt to begin with

@nikolausWest
Copy link
Member

With locked axis I can no longer scroll left-right with my trackpad. Also can't area-zoom by right click + drag (we talked about changing the default box zoom in this case so that the box would extend for the full y-range when y-axis is locked)

@Wumpf
Copy link
Member

Wumpf commented Feb 6, 2024

repros for me! when I tested I clicked on it, but track pad without clicking is broken indeed!

@Wumpf Wumpf added the do-not-merge Do not merge this PR label Feb 6, 2024
@Wumpf Wumpf self-requested a review February 6, 2024 09:35
@jleibs
Copy link
Member Author

jleibs commented Feb 6, 2024

With locked axis I can no longer scroll left-right with my trackpad. Also can't area-zoom by right click + drag (we talked about changing the default box zoom in this case so that the box would extend for the full y-range when y-axis is locked)

Ahh, didn't even realize trackpad horizontal scrolling was a thing I'm using a mouse where scroll is vertical and shift+scroll is horizontal. I disabled scrolling because I thought it was only vertical and leads to an annoying "bounce."

I also disabled the rectangular drag (noted in the ? menu) because there's not a way to make it full range without deep plot-surgery.

@jleibs jleibs removed the do-not-merge Do not merge this PR label Feb 7, 2024
Comment on lines +371 to +373
if lock_y_during_zoom {
ui.input_mut(|i| i.smooth_scroll_delta.y = 0.0);
}
Copy link
Member

Choose a reason for hiding this comment

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

doesn't that mean the scroll delta is zero everywhere?
hm, doesn't seem to affect anything else. okay

@Wumpf Wumpf merged commit 38a7d20 into main Feb 7, 2024
43 checks passed
@Wumpf Wumpf deleted the jleibs/axis_y branch February 7, 2024 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🟦 blueprint The data that defines our UI include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants