-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat #128: first step towards responsive layout #132
Conversation
const selectedLanguage = ref<FormLanguage>(); | ||
|
||
const initSelectedLanguage = () => { | ||
if(!props.form.currentState.activeLanguage.isSyntheticDefault) { | ||
selectedLanguage.value = props.form.currentState.activeLanguage; | ||
} | ||
} | ||
|
||
initSelectedLanguage(); |
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.
maybe do a check in parent component about isSyntheticDefault
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.
One thing that I didn't anticipate when we were chatting about this yesterday, but also worth mentioning: when a form is not translated, the mobile presentation currently moves the print button into a menu that has no other items. That print button takes up the same space as the hamburger, so it's not really saving any space.
I don't think we necessarily need to change anything on this now, but it's one more thing to add to the discussion of what to do with the print button in mobile. And what we do if/when there are more header controls like view toggle.
min-width: 0; | ||
|
||
> div { | ||
box-shadow: 0px 1px 2px 0px #0000004D; |
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.
move color codes to a global variable
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.
Add css variables for colors and box-shadow in theme.scss
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.
It's exciting to see this coming along! I have a few comments. I don't feel super strongly about most of them, and some probably warrant broader team discussion and possible UI/UX design iteration.
const selectedLanguage = ref<FormLanguage>(); | ||
|
||
const initSelectedLanguage = () => { | ||
if(!props.form.currentState.activeLanguage.isSyntheticDefault) { | ||
selectedLanguage.value = props.form.currentState.activeLanguage; | ||
} | ||
} | ||
|
||
initSelectedLanguage(); |
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.
One thing that I didn't anticipate when we were chatting about this yesterday, but also worth mentioning: when a form is not translated, the mobile presentation currently moves the print button into a menu that has no other items. That print button takes up the same space as the hamburger, so it's not really saving any space.
I don't think we necessarily need to change anything on this now, but it's one more thing to add to the discussion of what to do with the print button in mobile. And what we do if/when there are more header controls like view toggle.
</Card> | ||
</PrimeCard> | ||
|
||
<!-- for mobile and tablet --> |
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.
Also not something we need to act on right now (unless you want to explore it!): if we are sure we want to preserve the single-row presentation and condense horizontal space, this feels like a potentially excellent use case for container queries. Using it well for this might have other design implications (i.e. we'd need to know how much space is needed upfront before we can query it).
The nice thing about container queries here would be the possibility of triggering space-saving as needed, which might allow more functionality to stay visible for e.g. forms with shorter titles.
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.
Good suggestion. For now I have run with the existing media query, I will try to use it next time we have any changes in the UI of the Form header.
h1 { | ||
padding-left: 10px; | ||
text-overflow: ellipsis; | ||
white-space: nowrap; |
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.
We discussed this on the call yesterday, and I don't want to lose the context: if we're truncating text, we might want to think about how to support expanding it. There's potentially a component generalization to consider there.
Adding to that... do we actually want to truncate form title text? In terms of use of space, might it be a better user experience to let it flow, possibly with smaller font size.
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 have removed the truncation, instead Form title now wraps around.
color: #000; | ||
font-size: 1.5rem; |
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 color seems to affect only the hamburger icon, and doesn't seem to be dependent on screen size. Can we apply the color directly to that element (with a class identifying it as such)?
-
The font size does not seem to affect anything, as far as I can tell. The same size is applied with higher precedence on
.p-button.p-button-icon-only.p-button-rounded
.
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.
good catch, I have removed it.
&:hover{ | ||
background: var(--primary-100); | ||
} | ||
&:active, &:focus { | ||
background: var(--primary-50); | ||
} |
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.
Unclear where to put this comment, but this is where it's most visible: the hamburger menu looks great in its default state, but when these background colors are applied it shows the background all the way to the right edge of the viewport. This is a minor nitpick, but it feels a little unpolished and maybe worth a little finesse.
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 had a discussion with Aly and she suggested to add equal spacing on the right side as there is on the left side, which is 10px.
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.
Now that you mention this, it caught my eye that quite a lot of measurements in the styles are in px
. I don't think we should worry about this now, but I would like to give some thought into where relative units make sense for accessibility/scaling flexibility.
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.
Let's talk about this in Slack
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.
We talked about using rem
instead of px
in Slack and setting different font-size of root for different screen sizes. Tracking it in #143
packages/web-forms/e2e/vue.test.ts
Outdated
@@ -76,7 +76,7 @@ test('All forms are rendered and there is no console error', async ({ page, brow | |||
const langChanger = page.getByLabel('change language'); | |||
|
|||
if ((await langChanger.count()) > 0) { | |||
await langChanger.click(); | |||
await langChanger.first().click(); |
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 feels like something that could churn quite a lot. I'm generally not fond of littering UI with classes just for testing purposes, but it seems reasonable to make an exception for:
-
things that are global singletons (like this is)
-
E2E testing specifically, where it's already challenging to relate the test code to the intended structure it's targeting
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.
Added classes to make it easier to select language changer in e2e test.
* Dont show hamburger for mobile view if there is only print button. Show the print button directly. * Wrap the title instead of truncated it. * Add space to the right side of the print/menu icon for smaller screens. * Added css classes to language changer to make it easier to select in e2e test.
* Declared different colors as variables. * Removed show/back buttons from demo, added navigation via history API * Made FormLanguageDialog and FormLanguageMenu dumb components, state change is handled by parent
🦋 Changeset detectedLatest commit: 432a027 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
While making changes related to color codes, I went on a tangent and removed the back button from the demo and used history API for navigation. I wanted to remove |
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.
Looking great! I'm glad to land the demo navigation functionality here. As mentioned when we talked about it on a call, I had made a similar change in the ui-solid
demo at one point. I'd be cautious about expanding any kind of client navigation beyond the demo, especially using native APIs directly (they're notoriously error prone). Anyway, it feels especially nice to be able to reload (or make changes and see hot reload) without getting bumped back to the form list.
@@ -0,0 +1,5 @@ | |||
--- |
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.
When we discussed this, I said I don't think the file name matters. But I do wonder if it matters whether there's a .md
suffix. The changesets CLI always creates files with that suffix, and I noticed the same for the suggested filename when I used the GitHub flow in #135.
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.
renamed it.
</PrimeCard> | ||
|
||
<!-- for mobile and tablet --> | ||
<div class="flex lg:hidden align-items-center smaller-screens"> |
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 was curious to see how wrapping looks for forms with long titles. Pretty good! I think we may want to iterate later on...
-
More consistent alignment of controls (icons in larger views, hamburger menu in smaller). The vertical centering makes those a moving target. I think we'll probably want some design guidance on that.
-
Maybe slightly reducing text size/weight over a certain character length? (Honestly the weight feels pretty heavy regardless of length, wrapping.)
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.
Good points, I am quoting these in #139
&:hover{ | ||
background: var(--primary-100); | ||
} | ||
&:active, &:focus { | ||
background: var(--primary-50); | ||
} |
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.
Now that you mention this, it caught my eye that quite a lot of measurements in the styles are in px
. I don't think we should worry about this now, but I would like to give some thought into where relative units make sense for accessibility/scaling flexibility.
#128