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

Allow zoom buttons on small frames (with settings config) #1246

Merged
merged 4 commits into from
Dec 1, 2020

Conversation

ivnnv
Copy link
Contributor

@ivnnv ivnnv commented Nov 27, 2020

It seems that by historical reasons (#447) it is intentional to hide the zoom button on non-fullscreen frames., but I think nowadays we have big enough screen to have them visible (in the case the user wants, hence adding a settings option defaulting to false, just as the current behaviour).

So this PR is adding a config checkbox option to have the zoom buttons available and adapts those buttons to make them smaller accordingly.

zoom small frame

PS: CLA sign sent

@github-actions
Copy link

@OskarDamkjaer
Copy link
Contributor

Thanks for your PR and for signing the CLA!

I agree we should show the zoom controls more often and I don't even think we'd need to hide it behind a setting. It'd be reasonable to always show them.

Could you keep only the icon size change and remove the setting and the if (this.props.fullscreen || this.props.zoomOnSmallFrames) { check?

@OskarDamkjaer
Copy link
Contributor

@ivnnv Also, I added you to the allow-list so the CI should run your tests properly now.

@ivnnv
Copy link
Contributor Author

ivnnv commented Dec 1, 2020

@OskarDamkjaer friendly ping, I reworked the PR to remove the config setting and now all tests have passed and everything seems ready. Excited to have my first contribution merged!

@OskarDamkjaer
Copy link
Contributor

@ivnnv Looks great, thanks!

@OskarDamkjaer OskarDamkjaer merged commit 7896909 into neo4j:master Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants