Skip to content

Support for manual viewport size input & reintroduce resize key combinations #1146

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 23 commits into from
Closed

Support for manual viewport size input & reintroduce resize key combinations #1146

wants to merge 23 commits into from

Conversation

@JosefBredereck
Copy link
Contributor

@JosefBredereck JosefBredereck commented Apr 13, 2020

Closes

Issue Title
#1145 Viewport resizing

Summary of changes:

Features

  • Ability to change viewport width by changing the value manually

@JosefBredereck
Update object_factory and view all page behavior
2237301
@JosefBredereck
Various changes to file and pattern creation
edc4b0c 
- Fixed test cases incorrect
- Fixed view all pages not generated
- Fixed view all page could not be loaded
- Fixed pattern ordering
- Fixed subfolder patterns have invalid data
- Fixed subfolder patterns are not rendered properly
@JosefBredereck
Merge branch 'dev' of https://github.com/JosefBredereck/patternlab-node…
f9ca91e 
… into dev
@JosefBredereck
Ability for viewall page links and updated test to realworld
c94531b
@JosefBredereck
Fix "Open In New Tab" incorrect link
e39a881
@JosefBredereck
Add support for manual viewport size input
c9e99fd
@bmuenzenmeyer bmuenzenmeyer self-assigned this Apr 14, 2020
@sghoweri sghoweri self-assigned this Apr 14, 2020
@sghoweri
Copy link
Contributor

sghoweri commented Apr 14, 2020

Hey thanks so much for your help getting this original PL feature re-enabled @JosefBredereck!

Starting to take a look at this... 👀

@sghoweri sghoweri self-requested a review April 14, 2020 22:30
sghoweri
sghoweri requested changes Apr 14, 2020
View reviewed changes
Copy link
Contributor

@sghoweri sghoweri left a comment

Choose a reason for hiding this comment

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

Besides the changes in the pl-viewport-size-list.js file (which I'm on the fence on), there's a couple of things I'd recommend we iterate on to get this 100% up to speed (which FWIW is why this original PL feature wasn't yet re-enabled with the major UI refactor -- hadn't gotten around to buttoning up all of these interactions).

Let know if any of these (below) don't make sense or if there's anything here you feel strongly against (or think we should punt on) -- again, I really appreciate the help maintaining Pattern Lab's UI -- can't wait to get this quality of life feature re-enabled!!

Recommendations:

  • Clicking on the px text should focus on the input (maybe wrap the whole thing in a <label> tag?
  • We should try to update the base font size of the input field (at least on smaller screen sizes) to avoid having iOS (and Android?) zoom in while the input is focused... maybe change the input type while we're at it?
  • Unlike the original implementation of this functionality, you can no longer press the up or down arrow keys. Ideally the behavior I think we’d want to see here would be:
    • Pressing up or down on the arrow keys will increment / decrement the size by 1
    • Pressing shift + the up or down on the arrow keys would increment / decrement the viewport size by 10
  • Entering in a valid number into the input should automatically trigger the viewport to resize (or at the very least, entering a number into the input and tabbing away) — ie. I feel like users should be able to change the viewport size by simply changing the number (vs requiring them to hit enter)... 🤔
  • Deleting the text from the current viewport size and tabbing away should reset the displayed value to reflect the current viewport resized width
  • If the user inputs a value that’s outside of the min / max range of viewport sizes (and hits enter since that’s the current behavior on this PR branch I’m reviewing) — the displayed viewport size should correct itself to that min / max size vs displaying a value that’s out of bounds… Or to put it another way, the viewport resizer width should always be 100% in sync with the displayed size!

image

Side note: FWIW, I’ve never been a fan of the em option that displays next to the viewport size in pixels. That said, are users going to be confused by being able to (once again) set one input but not the other?

@JosefBredereck
Copy link
Contributor Author

JosefBredereck commented Apr 15, 2020

That's some valid points.

Unlike the original implementation of this functionality, you can no longer press the up or down arrow keys. Ideally the behavior I think we’d want to see here would be:

Entering in a valid number into the input should automatically trigger the viewport to resize (or at the very least, entering a number into the input and tabbing away) — ie. I feel like users should be able to change the viewport size by simply changing the number (vs requiring them to hit enter)

Yes and No. I also tested that behavior. But let's imagine you are at 500px and want to change it to 1000px. When you start to replace the number you start at 1px and the screen will be resized to 240px minimum. After that you go to 10px and 100px and nothing changes. Then you reach 1000px and the screen will jump to 1000px. You can come across with a debounce function - BUT - that will conflict again with changing the numbers by clicking up and down. Will the number be evaluated when the user is done with going up and down or do we have a different behavior for this functionality?

Deleting the text from the current viewport size and tabbing away should reset the displayed value to reflect the current viewport resized width

Should be fixed before merge.

If the user inputs a value that’s outside of the min/max range of viewport sizes (and hits enter since that’s the current behavior on this PR branch I’m reviewing) — the displayed viewport size should correct itself to that min/max size vs displaying a value that’s out of bounds… Or to put it another way, the viewport resizer width should always be 100% in sync with the displayed size!

Therefore we need Blur the input field when pressing enter so that the input value will start the sync again.

Side note: FWIW, I’ve never been a fan of the em option that displays next to the viewport size in pixels. That said, are users going to be confused by being able to (once again) set one input but not the other?

Actually that's something I was always wondering about. Or at least why that em is even there.

JosefBredereck
JosefBredereck commented Apr 15, 2020
View reviewed changes
packages/uikit-workshop/src/scripts/lit-components/pl-viewport-size/pl-viewport-size.js Outdated
<input
class="pl-c-viewport-size__input pl-c-viewport-size__input-action"
.value="${this.state.inputPixelValue}"
@keypress=${this.handlePixelUpdate}
Copy link
Contributor Author

@JosefBredereck JosefBredereck Apr 15, 2020

Choose a reason for hiding this comment

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

Also, I need to mention that I'm not that well known with React. Maybe there is a better approach for handling an input and replace/prevent an invalid key. In Angular, there are directives for that.

@JosefBredereck
Copy link
Contributor Author

JosefBredereck commented Apr 15, 2020

@sghoweri I added some more functionality. Please have a look at these too. 👍

@JosefBredereck JosefBredereck changed the title Add support for manual viewport size input [WIP] Support for manual viewport size input & reintroduce resize key combinations Apr 15, 2020
@bmuenzenmeyer
Copy link
Member

bmuenzenmeyer commented Apr 23, 2020

Side note: FWIW, I’ve never been a fan of the em option that displays next to the viewport size in pixels. That said, are users going to be confused by being able to (once again) set one input but not the other?

Actually that's something I was always wondering about. Or at least why that em is even there.

One of the primary use cases of the viewport resizer is to exactly test media queries.

I can almost certainly guarantee em is there because of this: https://css-tricks.com/em-based-media-queries-are-based-on/

Chris makes it sound as if this is no longer needed, which TIL.

@JosefBredereck JosefBredereck changed the title [WIP] Support for manual viewport size input & reintroduce resize key combinations Support for manual viewport size input & reintroduce resize key combinations Apr 25, 2020
@JosefBredereck
Copy link
Contributor Author

JosefBredereck commented Apr 28, 2020

Oh no, I merged the wrong dev branch here 😢

@JosefBredereck
Copy link
Contributor Author

JosefBredereck commented Apr 28, 2020

It can not be saved, I close this one and make a new 🤦

@JosefBredereck JosefBredereck mentioned this pull request Apr 28, 2020
Merged
@JosefBredereck JosefBredereck deleted the feature-viewport-input-sizing branch April 28, 2020 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bmuenzenmeyer bmuenzenmeyer bmuenzenmeyer left review comments

@sghoweri sghoweri sghoweri requested changes

@raphaelokon raphaelokon Awaiting requested review from raphaelokon

Assignees

@bmuenzenmeyer bmuenzenmeyer

@sghoweri sghoweri

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JosefBredereck @sghoweri @bmuenzenmeyer