-
Notifications
You must be signed in to change notification settings - Fork 92
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
Reduce border radius of components #5483
Conversation
Wouldn't it be quite a major design change for v8? |
@ShGKme can't we release a new major? |
It is already released as alpha, migrated to Vue 3. Though some apps are already migrated ( Another alternative is to release v9 with a design change and move Vue 3 support to v10. But then we need to support three branches of Or we can just make this design change in patch releases of Nextcloud 28 and 29. |
One idea I have — a hacky workaround here could be to define a new temporal variable on server, aka, // Server
:root {
// Nextcloud 28 | no --border-radius-new-design
// Nextcloud 29+ | temp variable to use old design
--border-radius-new-design: var(--border-radius-large);
// Nextcloud 3x | @nextcloud/vue@9 and Vue 3, new var not needed anymore
}
// @nextcloud/vue
.NcListItem,
.NcButton,
.NcAppNavigationItem {
// @nextcloud/vue@8 | use new value if available (on 29+), but fallback to the look-like-old one (on 28)
border-radius: var(--border-radius-new-design, 50%);
// @nextcloud/vue@9 | no need to hack
border-radius: var(--border-radius-large);
} |
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.
Design-wise it looks very nice, great work @marcoambrosini! :) 2 details:
- The reactions in Talk are such small elements but their radius is still comparatively large, maybe that would be better as e.g. 4px (grid baseline)?
- Labels like in Mail and the avatarbubble could keep their pill-style, I assume that’s still the case as they are not among the elements changed?
@ShGKme just to clarify: This change is planned for the next major, v30. Would just waiting a bit to merge this change resolve the issues? I don’t think it would be a good idea to backport this to 28 and 29.
@jancborchardt the problem is that currently next major (v9) is planned to be the Vue3 migration (already alpha version available). But not all apps might migrate to Vue3 for Nextcloud 30, so for breaking changes like this we might need to spin up a different major version for Vue2, then we will have to maintain:
This might be a lot of work. |
@AndyScherzinger in case you have a good course of action. |
I would favor keeping those rounded along with labels and avatar chips, wdyt @jancborchardt ? |
What about keeping it as planned:
And when 30 comes out, we also do a V8 minor that incorporates these changes? I think that if it saves a lot of work, it might be worth it |
Yeah sounds good to me too, so pill-style for those too yeah? |
I like this UI changes ! I did find some elements too rounded. |
Not really. I need to rely on the frontend peeps here in terms of ideas. Best case there would be a way from an app perspective to "choose" new/old border radius like mentioned by @ShGKme or we gone down the road of #5483 (comment) mentioned by @susnux. In general while visually a change I expect the UI elements to keep having the very same size in terms of "pixel by pixel". So we could also consider it a non-breaking change. So not sure if possible, but having "just" 2 lib versions one for vue2 and one for vue3 but the ability to choose between the radius size would be less maintenance work in terms of number of lib versions to maintain. |
I'd say it depends on the number of breaking changes we want to do before Vue 3 migration. If it is only about this border radius, then the problem is solvable by a 1-line change in the server styles without having to support 3 branches of the |
@ShGKme there will be other changes to refresh the UI a bit, for example input fields will likely change, some borders and border radii here and there. |
Btw having 3 versions to support is only a problem for the first ~2 months, because after that we not doing that much for old versions (see stable7) |
@susnux so what should I do here? |
Do you have any examples of input fields changes or what is going to be change "here or here"? Is it something more than theming? If there are plans for technically large changes, we have no other choice than break v9 beta to move it to v10 and make a new v9 major release. If there are no, then we can manage it via theming. without making a new version. |
Inputs and other elements' borders will become 1px instead of two, possibly transparencies will be added to some elements. We do not know the full scope of the changes yet, but the idea is to freshen up the UI a bit for the next release. |
e31dc68
to
26f85e9
Compare
Doable with theming
But this is not very specific 😀 IMO, from the technical side, it'd be easier to discuss when the changes are more determined 👀 |
Another side of the version branch issue. Do we expect apps running on multiple Hub versions (e.g. groupware apps) to have new design on old Hub versions (using new |
@ShGKme I would say the latter |
Signed-off-by: Marco Ambrosini <marcoambrosini@proton.me>
1b6dadd
to
13a6ae7
Compare
Not breaking anymore with the fallbacks in place, right? |
Yes :) |
/backport to next |
Use border radius large for NcAppNavigationItem, NcListItem and NcButton