Skip to content
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

Merged
merged 1 commit into from
May 27, 2024
Merged

Conversation

marcoambrosini
Copy link
Contributor

@marcoambrosini marcoambrosini commented Apr 11, 2024

Use border radius large for NcAppNavigationItem, NcListItem and NcButton

Screenshot 2024-04-11 at 10 23 56 Screenshot 2024-04-11 at 10 23 05 Screenshot 2024-04-11 at 10 32 12 Screenshot 2024-04-11 at 10 37 48

@marcoambrosini marcoambrosini self-assigned this Apr 11, 2024
@marcoambrosini marcoambrosini requested review from susnux, a team, skjnldsv and ShGKme April 11, 2024 08:35
@ShGKme
Copy link
Contributor

ShGKme commented Apr 11, 2024

Wouldn't it be quite a major design change for v8?

@marcoambrosini
Copy link
Contributor Author

@ShGKme can't we release a new major?

@ShGKme
Copy link
Contributor

ShGKme commented Apr 11, 2024

@ShGKme can't we release a new major?

It is already released as alpha, migrated to Vue 3.

Though some apps are already migrated (tasks) or preparing to the migration (groupware, talk), I'd not expect all the apps, especially files/server to finish migration to Vue 3 = @nextcloud/vue@9 in Nextcloud 30...

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 @nextcloud/vue. I'd like to avoid that 🥲

Or we can just make this design change in patch releases of Nextcloud 28 and 29.

@ShGKme
Copy link
Contributor

ShGKme commented Apr 11, 2024

One idea I have — a hacky workaround here could be to define a new temporal variable on server, aka, --border-radius-new-design (I'm bad with naming), use it in the components and define on the server. Use it in @nextcloud/vue@8 and fallback to the look-like-old value. Then the server will control this design change until the migration to @nextcloud/vue@9 with Vue 3.

// 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);
}

Copy link
Contributor

@jancborchardt jancborchardt left a 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.

@susnux
Copy link
Contributor

susnux commented Apr 11, 2024

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:

  • v8 for 28 and 29
  • v9 for 30 with Vue2
  • v10 for 30 with Vue3

This might be a lot of work.

@jancborchardt
Copy link
Contributor

@AndyScherzinger in case you have a good course of action.

@marcoambrosini
Copy link
Contributor Author

marcoambrosini commented Apr 11, 2024

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)?

I would favor keeping those rounded along with labels and avatar chips, wdyt @jancborchardt ?

@marcoambrosini
Copy link
Contributor Author

marcoambrosini commented Apr 11, 2024

What about keeping it as planned:

  • V8 for 28 and 29 and 30 with vue2
  • V9 for 30 with Vue 3

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

@jancborchardt
Copy link
Contributor

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)?

I would favor keeping those rounded along with labels and avatar chips, wdyt @jancborchardt ?

Yeah sounds good to me too, so pill-style for those too yeah?

@Jerome-Herbinet
Copy link
Member

I like this UI changes ! I did find some elements too rounded.

@AndyScherzinger
Copy link
Contributor

@AndyScherzinger in case you have a good course of action.

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.

@ShGKme
Copy link
Contributor

ShGKme commented Apr 11, 2024

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 @nextcloud/vue

@marcoambrosini
Copy link
Contributor Author

If it is only about this border radius

@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.

@susnux
Copy link
Contributor

susnux commented Apr 11, 2024

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)

@marcoambrosini
Copy link
Contributor Author

@susnux so what should I do here?

@ShGKme
Copy link
Contributor

ShGKme commented Apr 23, 2024

@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.

@marcoambrosini
Copy link
Contributor Author

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.

@Jerome-Herbinet Jerome-Herbinet changed the title Recduce border radii of navigation items and buttons Recduce border radius of navigation items and buttons Apr 24, 2024
@ShGKme ShGKme changed the title Recduce border radius of navigation items and buttons Reduce border radius of navigation items and buttons Apr 24, 2024
@ShGKme
Copy link
Contributor

ShGKme commented Apr 29, 2024

Inputs and other elements' borders will become 1px instead of two

Doable with theming

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.

But this is not very specific 😀

IMO, from the technical side, it'd be easier to discuss when the changes are more determined 👀

@marcoambrosini
Copy link
Contributor Author

@ShGKme can't we just maintain the 3 version for 2 months as @susnux mentioned?

@skjnldsv skjnldsv added 2. developing Work in progress design Design, UX, interface and interaction design labels Apr 30, 2024
@ShGKme
Copy link
Contributor

ShGKme commented May 12, 2024

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 nextcloud-vue major) or old design more consistent with the server (using variables)?

@marcoambrosini
Copy link
Contributor Author

@ShGKme I would say the latter

@marcoambrosini
Copy link
Contributor Author

@ShGKme @susnux what should I do here then? Move it to the theming app?

@marcoambrosini
Copy link
Contributor Author

@ShGKme @susnux I switched to variable mode and added the fallbacks, please review 🙏

Signed-off-by: Marco Ambrosini <marcoambrosini@proton.me>
@marcoambrosini marcoambrosini added 3. to review Waiting for reviews and removed 2. developing Work in progress breaking PR that requires a new major version labels May 22, 2024
@marcoambrosini
Copy link
Contributor Author

Not breaking anymore with the fallbacks in place, right?

@susnux
Copy link
Contributor

susnux commented May 22, 2024

Not breaking anymore with the fallbacks in place, right?

Yes :)

@marcoambrosini marcoambrosini merged commit afc6c31 into master May 27, 2024
18 checks passed
@marcoambrosini marcoambrosini deleted the reduce-border-radius branch May 27, 2024 09:05
@susnux
Copy link
Contributor

susnux commented May 28, 2024

/backport to next

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Design, UX, interface and interaction design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants