-
Notifications
You must be signed in to change notification settings - Fork 60
fix(talk/settings): adjust to new design #1530
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
Yeah, that immediately jumped my eye as well. Should stay with a dropdown there I think |
de19227 to
b94f779
Compare
b94f779 to
4346729
Compare
Antreesy
left a comment
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.
Otherwise nice splitting
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.
Shall be better exposed in vue-lib, I guess?
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.
Exposing it would require keeping its API. I'd avoid it until we know well it works.
In this specific case it can be removed after:
| </NcButton> | ||
| </NcFormBox> | ||
|
|
||
| <NcButton :class="itemClass" wide @click="modelValue = 1"> |
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.
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.
Pushed an update
| import SettingsFormGroup from './components/SettingsFormGroup.vue' | ||
| import SettingsSelect from './components/SettingsSelect.vue' | ||
| import SettingsSubsection from './components/SettingsSubsection.vue' | ||
| import { ZOOM_MAX, ZOOM_MIN } from '../../../constants.js' |
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.
Are these constants still used?
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.
Yes, on the backend
|
Updated Zoom setting, see screenshots |
|
|
||
| <script setup lang="ts" generic="T extends string | null"> | ||
| import { computed, useId, useTemplateRef } from 'vue' | ||
| import IconUnfoldMoreHorizontal from 'vue-material-design-icons/UnfoldMoreHorizontal.vue' |
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.
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 would use the arrow bottom that we use in the NcSelect components
There is no way to detect the open state to make it UP on open like in a fully custom select.
That is why unfold (arrow up + down) is often the default select icon.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
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.
Looks quite nice. Some smaller points of feedback:
- The reset split button could have a bit more left/right padding, enough to align the icon with trailing icons in
FormBox*components. - The two options related to secondary speakers should probably have their own group to indicate that they are more related to each other than any other two notification options. Even better would be if they could be in the same box, but not sure if that works with the split button.
- "Notifications and sounds" could be renamed to "Notifications & Sounds" for consistency
- "Theme" could be moved below "Appearance" for structure.
- The zoom controls are indeed a bit tricky. For now, I'm thinking:
This gets rid of the slider but it makes it so you don't need to move your mouse too much to click on plus after you clicked on minus and vice versa.
It would get rid of the keyboard shortcuts, but that's probably fine? Ideally, they would be in the actual Keyboard shortcuts section anyway, but I'm guessing we don't want to insert them there?
This is something I'd be willing to revisit later by the way. But probably worth it only after we move to Material Symbols. |
🥲👍 (Needs more hacking or new components or making our buttons FormBox compatible by default/additionally)
👍
👍
Possible with some additional work, will do it later. Though, makes more sense if we have many desktop-specific shortcuts.
It cannot, because from the new settings design structure, a radio group has the same structuring level as a general group (it is also a group). Level 1: Settings Section (labelled)
Then:
|
Sorry, I meant below as in just swapping their positions, not putting one inside the other. It's still more structured that way if a more general thing is above a more specific thing.
If we do increments of 5%, that is probably fine. The difference between 103% and 105% is not very relevant. The difference between 100% and 105% could be.
Pf, I understand what you mean. I was thinking of for example Firefox, where you can click on the percentage to reset the zoom… maybe it is fine to change the a11y label/description manually here to accommodate this? I'm guessing that's what would be done if there was no original "Zoom" title in the UI and it was just the percentage (which was my original idea, I just decided that the title could be put here).
It doesn't look too bad haha, it's just that in this particular case, where it zooms the UI, I'm not sure how useful it is anyway, because:
And as I said before, adjustments like 103% vs. 105% aren't that different and people would probably want a more round number anyway, so for coarser adjustments, I think a button makes more sense too. |
Then if I want 400%, I need to click 60 times |
That is why it has an internal value, which only applies zoom once you released the mouse, but not while you are zooming. So this case is explicitly handled. |
c36c8b4 to
0960176
Compare
|
To be clear: I am not advocating for my proposal in terms of visuals. There should never be a need to give up functionality in favor of it. I'm only advocating for it from a practical POV:
You raised a good point about it being difficult to jump big ranges in my initial proposal. I think with a dropdown and the buttons, we wouldn't lose anything by going with the more straightforward design and we would gain clarity/ease of use. With that said, I do not think the slider looks particularly good, no. But if you choose to go ahead with it, I would try to adjust it to fit it into the style better. Please let me know what you think. |
Not semantically, but generally, it does. This type of organization is still an important part of the part of layouts like this imo and I used similar thinking to organize other settings dialogs.
I did also think about theme being a more common option, but I thought that it has such a distinctive selector and it would still be pretty high up (you never have to scroll to see it) that it wouldn't be an issue. |
They used to be really far (on the opposite sides of the slider), but now they are on the sides of the input, quite close, aren't they?
I can add the start, end and 100% labels
Regarding Zoom, I'm fine removing the slider, but I don't want to lose the manual input to have an exact zoom you need.
We'll have more "Appearance" settings later (soon), including tray-related |
Dismissing to move forward with something that should not be discussed this long
That's fine for now as long as the label is aligned (as I've been nagging you about in DMs, sorry!)
It being wide is OK. If there's gonna be whitespace either way, it might as well increase the click target instead of contribute to the empty void :)
Oh no, as I said earlier, if we go with a dropdown, I would show only larger fixed, amounts in there. Browsers and other apps do this with increments. So for example: 50% Then the buttons could still adjust the zoom level by 5% increments for finer adjustments.
You know, that's fine by me, I like it. I'm still not entirely sure about the reset button as you can just select 100%, but it looks fine… And if there is no reset button, I'd also move the edit button to the right to keep the text aligned. But your call, really. The only question: Would the edit button edit the value in-line? Would it spawn a dialog or just an input field nearby? |
It had a different change for a reason. Moving from 50% to 100% (+50%) makes everything twice as large, which is a significant change. In the web-browsers it is always annoying that you can Zoom with a mouse/keyboard, and the zooming in is slower and slower while zooming out is faster and faster. That is why we use the factor. |
Inline, replacing the select |
Can I see how that looks? |
Ok, that is very fair, it just indeed doesn't look very nice and people might want to match their zoom level for the app to one offered by the browser which usually round them to 5. How about keeping them roughly the same, just rounding them a bit? This would mean the increments would be 5, 10, 15, 25: |
Signed-off-by: Grigorii K. Shartsev <[email protected]>
Signed-off-by: Grigorii K. Shartsev <[email protected]>
Signed-off-by: Grigorii K. Shartsev <[email protected]>
0960176 to
fe9c156
Compare
Screenshot is updated. |
|
@Antreesy @DorraJaouad If you want to have a look again - let me know. Otherwise you can merge 👀 |
Antreesy
left a comment
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.
Code-wise OK, zoom value is not limited to min and max (+- buttons can still change it)
Signed-off-by: Grigorii K. Shartsev <[email protected]>
fe9c156 to
833ebc8
Compare
| <NcFormBox class="zoom-box__row" row> | ||
| <UiFormBoxSplitButton | ||
| :label="t('talk_desktop', 'Zoom out')" | ||
| hide-label | ||
| :disabled="modelValue / STEP_FACTOR < ZOOM_MIN" | ||
| @click="modelValue /= STEP_FACTOR"> | ||
| <template #icon> | ||
| <IconMinus :size="20" /> | ||
| </template> | ||
| </UiFormBoxSplitButton> | ||
|
|
||
| <UiFormBoxSplitButton | ||
| :label="t('talk_desktop', 'Zoom in')" | ||
| hide-label | ||
| :disabled="modelValue * STEP_FACTOR > ZOOM_MAX" | ||
| @click="modelValue *= STEP_FACTOR"> | ||
| <template #icon> | ||
| <IconPlus :size="20" /> | ||
| </template> | ||
| </UiFormBoxSplitButton> |
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.









☑️ Resolves
NcSelectfor Nextcloud 30 design is not implemented yetZoom is draft - I need to think about itRadio groups don't look well with long text even on a normal screen🖼️ Screenshots