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

Use large corner radius for the Popover component #2566

Merged

Conversation

quentinguidee
Copy link
Contributor

This PR tries to takes a further step towards the standardization of border radii. Currently, border radii are not uniform. Among other things, buttons have a very high border-radius, while Popovers are only allowed a 3px border-radius, and lists have none.

In the current state of this PR, here are some examples:

Before After
check-1 check-2
menu-1 menu-2
large-1 large-2

Another example (done with devtools) of what it looks like in-app:

Capture d’écran 2022-03-22 à 15 49 50

In my opinion, it gives a more modern look to NextCloud.

To improve this PR, we could add some vertical padding, but this is open to discussion.

Signed-off-by: Quentin Guidée <quentin.guidee@gmail.com>
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.

Looks much nicer, and I think @marcoambrosini will like this too. :)

@quentinguidee 2 things:

  • The whitespace chars/tabs seems off in your edit
  • What is the overflow:hidden for?

@quentinguidee
Copy link
Contributor Author

quentinguidee commented Mar 22, 2022

Without the overflow: hidden, when we hover an item, it overflow on the corners (you can even check that behaviour in NC for the moment, the 3px border-radius "comes back to 0" when hovering the first or the last item).

For the whitespace chars/tabs, I'm not sure to understand

Edit : an alternative to the overflow: hidden is to apply border-radius to the first and the last item, but I don't know if it's necessary and/or if it's a good idea

@marcoambrosini marcoambrosini mentioned this pull request Mar 23, 2022
@ChristophWurst
Copy link
Contributor

@jancborchardt could you please specify what major versions of Nextcloud should have this styling? E.g Calendar support Nextcloud 24 down to Nextcloud 21. If we upgrade to this change then installations of Nextcloud 21 will also see rounded corners for Calendar, but not for Talk. I'll let you judge if this is a breaking design change.

@marcoambrosini marcoambrosini merged commit 66bf198 into nextcloud-libraries:master Mar 23, 2022
@ChristophWurst
Copy link
Contributor

@marcoambrosini the Talk app only targets one major version of Nextcloud at a time, right? So you wouldn't update @nextcloud/vue for the old major version that targets Nextcloud 21, right?

Compare https://github.com/nextcloud/spreed/blob/master/appinfo/info.xml#L55 to https://github.com/nextcloud/calendar/blob/main/appinfo/info.xml#L36.

@jancborchardt
Copy link
Contributor

@ChristophWurst seems fine by me to have this in Calendar too, also for lower. Nextcloud versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants