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
T262373: add gallery focus mode #86
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solution looks good and makes sense to me 👍
src/gallery/slider.js
Outdated
@@ -260,6 +315,30 @@ const clientWidth = window.innerWidth, | |||
} ) | |||
}, | |||
|
|||
toggleFocusMode = () => { | |||
const toggleElements = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider only flipping a class here and apply the proper show/hide in CSS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a good idea, let me look into that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about ba7d7f7?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the class should be put higher up on a single element. Which elements should be shown or hidden can be handled in CSS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I think I see what you meant with your first comment, check out 3248fae. I like this much better, fully takes advantage of the cascading property of CSS to simplify and:
- get rid of the nested looping,
- get rid of
focusMode
state variable - make it more readable and maintainable
Thanks for the great feedback!
I have merged the latest master to this branch, easier for us to review the changes code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 !
https://phabricator.wikimedia.org/T262373
The gallery focus mode allows users to hide all other information in the gallery with a single tap to only display the image itself.
Note this PR builds on top of #85, hence the commit list showing all commits. The only commit relevant to focus mode at this point is 26d2743.
Let's aim to merge #85 first.