-
-
Notifications
You must be signed in to change notification settings - Fork 199
Mobile panel toggle buttons #1330
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
Conversation
…moreinfo Mobile trigger moreinfo
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@jamesmisson and @LlGC-jop, I think it will be easier for me to test/review/evaluate this when the CSS is updated and the work from #1325 is merged in. Do we want to try to do any incremental merges, or are we better off merging all of this into one epic PR? |
"feedback": "Feedback", | ||
"fullScreen": "Full Screen", | ||
"moreInfo": "More Information", | ||
"toggleLeftPanel": "Toggle Left Panel", |
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 it's a bug that this file is missing the shorthand variables for translations. This should probably be $toggleLeftPanel, and the other text should be converted as well (maybe that's a separate PR!).
"open": "$open", | ||
"share": "$share" | ||
"share": "$share", | ||
"toggleLeftPanel": "Toggle Left Panel" |
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.
All of these other instances should be converted to use $toggleLeftPanel like the uv-av-extension.
"$feedback": "Adborth", | ||
"$fullScreen": "Sgrin lawn", | ||
"$moreInfo": "Mwy o Wybodaeth", | ||
"$toggleLeftPanel": "Toggle Left Panel", |
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.
...and we should be sure not to forget about getting translations for this!
@jamesmisson, Thanks for the great work on this so far! The toggle for the left panel to access thumbnails index looks good in mobile view. I did have a small thought, when I tap on an image, I was expecting it to open in the centre panel so that I could zoom in, zoom out and read more easily. Also, when I return to the thumbnail view, it would be great if the image(or page) I had selected remains highlighted, similar to how it works on a desktop. |
@LanieOkorodudu, I think when the work here and the work in #1325 are integrated, the mobile experience will make more sense. I believe @LlGC-jop and @jamesmisson will be integrating their work into a new PR later today -- when that is done, the result should be more meaningful to test than these isolated parts. |
Superseded by #1343. |
NB this PR replaces #1321 so I've closed that one (which was just for the right button but used a confusing variable name for the toggle right panel event).
This one adds both buttons on the mobile footer that toggle the left and right panels open/closed. To do this I've: