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

Zoom in plots #1592

Merged
merged 9 commits into from
Apr 21, 2022
Merged

Zoom in plots #1592

merged 9 commits into from
Apr 21, 2022

Conversation

sroy3
Copy link
Contributor

@sroy3 sroy3 commented Apr 20, 2022

As mentioned in #1254

Screen.Recording.2022-04-20.at.10.50.12.AM.mov

I also tried adding the zoom to the comparison table, but the fact that it's an image, it's already most likely at its full size and seeing it take only 30% of the screen while zooming makes the zoom feel broken.

@sroy3 sroy3 added the product PR that affects product label Apr 20, 2022
@sroy3 sroy3 self-assigned this Apr 20, 2022
@sroy3 sroy3 marked this pull request as ready for review April 20, 2022 17:53
<div
className={styles.backdrop}
onClick={onClose}
role="none"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for role="none" is that this isn't the main way to close the modal. I don't think adding a keyboard event would really be useful here.

Copy link
Contributor

Choose a reason for hiding this comment

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

For me esc to close would be handy.

Copy link
Contributor

@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 really good.

A couple of minor things for follow up IMO.

I think this behaviour is fine but would be good if the X didn't disappear:

Screen.Recording.2022-04-21.at.10.41.26.am.mov

.modal {
background-color: $bg-color;
position: relative;
border: 1px solid $fg-color;
Copy link
Contributor

Choose a reason for hiding this comment

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

[C] Personally I'm not a fan of this border. Could we have a shadow instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried a light and focusBorder shadows and they both look terrible. On dark themes, dark shadows are not really visible and no borders make it hard to see what is the modal and what isn't. I also didn't want to have a border, but that's really the only thing that worked.

I'll try finding a different modal background colour and I think that later I'll create a story in Storybook that will show every colour from the CSS variables. I'll also load a few themes for references. That would make it easier to select colours in general and follow the theme better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen.Recording.2022-04-21.at.9.15.42.AM.mov

Not perfect in every theme, but I that is the best variable I found.

@codeclimate
Copy link

codeclimate bot commented Apr 21, 2022

Code Climate has analyzed commit a8beda4 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 96.8% (0.0% change).

View more on Code Climate.

@sroy3
Copy link
Contributor Author

sroy3 commented Apr 21, 2022

This is really good.

A couple of minor things for follow up IMO.

I think this behaviour is fine but would be good if the X didn't disappear:

Screen.Recording.2022-04-21.at.10.41.26.am.mov

Fixed. Didn't know the viewport just gets hidden when going too small. It will disappear when extremely small, but when the viewport is that small, you can't see anything else either.

@sroy3 sroy3 merged commit c1b54d3 into main Apr 21, 2022
@sroy3 sroy3 deleted the zoom-plots branch April 21, 2022 13:24
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