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

Make the plot sizing sliders sticky #3443

Merged
merged 4 commits into from
Mar 13, 2023
Merged

Make the plot sizing sliders sticky #3443

merged 4 commits into from
Mar 13, 2023

Conversation

sroy3
Copy link
Contributor

@sroy3 sroy3 commented Mar 10, 2023

Part of #2585

Screen.Recording.2023-03-10.at.2.31.13.PM.mov

@sroy3 sroy3 added the product PR that affects product label Mar 10, 2023
@sroy3 sroy3 self-assigned this Mar 10, 2023
@sroy3 sroy3 marked this pull request as ready for review March 10, 2023 19:39
@shcheklein
Copy link
Member

I think sliders should be part of the header with the section name (is it sticky now?). We should be trying to optimize the space here. Same as we do with buttons that control the section.

Copy link
Member

@mattseddon mattseddon left a comment

Choose a reason for hiding this comment

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

This is an improvement. I agree with what Ivan said. The section header should be condensed and sticky.

Side note: Stories/snapshots in Chromatic look really strange with the slider moved to the LHS.

@sroy3
Copy link
Contributor Author

sroy3 commented Mar 13, 2023

I think sliders should be part of the header with the section name (is it sticky now?). We should be trying to optimize the space here. Same as we do with buttons that control the section.

They have to be separate as the sliders cannot be in the header when the section is collapsed. But they can behave the same way. I'll make the section header sticky as well in a follow-up.

@sroy3
Copy link
Contributor Author

sroy3 commented Mar 13, 2023

This is an improvement. I agree with what Ivan said. The section header should be condensed and sticky.

Side note: Stories/snapshots in Chromatic look really strange with the slider moved to the LHS.

Can't seem to figure out why. It looks ok in Storybook and the slider was on the left on main screenshots as well. I'll try to fix this when merging inside the vertical sizer as this requires the CI to test.

@sroy3 sroy3 enabled auto-merge (squash) March 13, 2023 14:15
@codeclimate
Copy link

codeclimate bot commented Mar 13, 2023

Code Climate has analyzed commit 39ec522 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 95.7% (0.0% change).

View more on Code Climate.

@sroy3 sroy3 merged commit f0f6716 into main Mar 13, 2023
@sroy3 sroy3 deleted the sticky-size-sliders branch March 13, 2023 15:37
@shcheklein
Copy link
Member

They have to be separate as the sliders cannot be in the header when the section is collapsed. But they can behave the same way. I'll make the section header sticky as well in a follow-up.

We can hide the controls when it's collapsed. I still think it's better to take less space. It's hard to justify 3 sticky rows (ribbon + header + controls).

@sroy3
Copy link
Contributor Author

sroy3 commented Mar 13, 2023

They have to be separate as the sliders cannot be in the header when the section is collapsed. But they can behave the same way. I'll make the section header sticky as well in a follow-up.

We can hide the controls when it's collapsed. I still think it's better to take less space. It's hard to justify 3 sticky rows (ribbon + header + controls).

I'll try to move them inside the header while making the headers sticky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR that affects product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants