-
Notifications
You must be signed in to change notification settings - Fork 407
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
Support for manual viewport size input & reintroduce resize key combinations #1167
Support for manual viewport size input & reintroduce resize key combinations #1167
Conversation
- Via manual input - Via shortcuts
IMO, the following is still needed:
|
Eiter way I need acknowledgment of everybody that both ways are ok a good aproach. Or which one to take. Em or no em thats the question 😅 |
@bradfrost do you have thoughts on the current state of using |
Maybe I misunderstood that in the first place. I implemented to reset the value to the current size. But you meant to update the viewport size on blur, right? |
yes, but that's just my opinion. someone with better UX chops or experience can certainly overrule me. |
Almost every codebase I've encountered over the last number of years uses I'm flying blind here and unfortunately don't have the bandwidth to pull this down and look at it, but my strong preference would be to retain the existing functionality. That functionality is:
Does that make sense? Again, sorry I don't have time to get hands on with this, but let me know if you have questions. |
- Revertet overflow-x value previously setted because it causes an error for the resize bar
Thank you @bradfrost for me it's clear now. I implemented also the ability to edit also the em value. Also, I implemented the ability for up and down keys in the following way: If you click up/down the value for px will go up/down by 1px. If you click up/down the value for em will go up/down by 0.1em. This PR is now ready for review @sghoweri 😃 |
Thanks @JosefBredereck! I'll check this out later today or tomorrow (worst case)! |
@@ -10,7 +10,7 @@ import Mousetrap from 'mousetrap'; | |||
import VisuallyHidden from '@reach/visually-hidden'; | |||
import Autosuggest from 'react-autosuggest'; | |||
|
|||
import { urlHandler } from '../../utils'; | |||
import { urlHandler, iframeMsgDataExtraction } from '../../utils'; |
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.
+1 on moving this logic out out into utils
. Been itching to do this but hadn't gotten around to it!
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 still need to double-check the updated viewport resizer buttons + functionality (ex. hay
mode) but wanted to flag an issue I'm noticing...
I can't seem to enter a specific size in the viewport resizer input field (ex. 600) via keyboard and by tabbing away (or by hitting enter on the keyboard) have that new value get used.
Pressing up / down on the keyboard + up / down with shift all works great though. I just can't seem to manually type in a value that'll get used. 🤔
Yes, this behavior is intended. (But enter should work) As brad frost said in the following
Also, if you have some insights on handling stuff in react better than I did I would really appreciate to get hints and quirks of that framework. |
I see, found the bug. It seems to work on the first two entries and then it breaks. Will debug and find this one. |
@sghoweri I was able to fix this issue. The problem is that some values can only be checked at keypress, others at key down and also some others at key up 🙈 |
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.
This passes a check of the default functionality found on the handlebars preview, and looks to address all of Brad's feedback.
Nice work @JosefBredereck !
I'll defer the merge to @sghoweri feels satisfied - he owns the UI.
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.
Overall this looks really great -- very nice work @JosefBredereck!
Tested locally (including with disco, random, and hay modes enabled) and at first glance, everything seems to be working as expected.
There's just two small things I still think need to get addressed however I'd recommend we tackle these separately and get the bulk of this work merged down ASAP.
- @bmuenzenmeyer's earlier comment, "blurring out of an input should update the value" is still outstanding as far as I can tell.
Currently users have to hit enter after typing in a value in the input field whereas I feel like typing in a valid value and blurring away should also trigger the viewport to be resized.
- Based on testing this out on my phone, it looks like the input field still zooms in when the input is focused on. Not a deal breaker but would definitely be something I'd want to button up.
Thanks again for all the work putting this together! 🎉
After testing the preview I'm ok with blurring the input reverting back to the set value too. |
lol - put me down as "on the fence" for this one. I see some UX pros and cons to both approaches. Not a deal breaker here so we'll keep iterating on these small micro-interactions. |
I will make a test on the weekend with my phones also. Maybe I can do anything against that. |
@sghoweri I fixed the zooming problem |
@JosefBredereck thanks! Taking another look now. |
Works great 👍 - lets get this merged. |
- Revertet overflow-x value previously setted because it causes an error for the resize bar
…e-viewport-and-shortcuts Support for manual viewport size input & reintroduce resize key combinations
Info
This one is a re-request of #1146 because of a wrong merge the other branch couldn't be saved. See the conversation on the other ticket for further information and topics.
Closes
Summary of changes:
Features