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

bug: border radius not set on elements that have no p- class #1342

Closed
MentalGear opened this issue Nov 10, 2022 · 9 comments
Closed

bug: border radius not set on elements that have no p- class #1342

MentalGear opened this issue Nov 10, 2022 · 9 comments

Comments

@MentalGear
Copy link
Contributor

MentalGear commented Nov 10, 2022

Noticed the following bug where border-radius is not set when the overlying tag does not include p-0.

Screen.Recording.2022-11-10.at.11.36.17.mov

Seems like this is the culprit:

Screenshot 2022-11-10 at 11 38 54

On a sidenote: Does daisyui use visual testing ?

@saadeghi
Copy link
Owner

saadeghi commented Nov 10, 2022

I will check the padding issue and I will update this issue.
About visual testing, there are no tests to compare each component based on their pixel measurements because that's unpractical in my opinion. Each component's visual representation depends on the theme, fonts and other factors but there is an accessibility test to make sure each color combination on each theme meets a required contrast value: https://github.com/saadeghi/daisyui/blob/master/src/tests/a11y.test.js

@saadeghi
Copy link
Owner

Fixed in latest version.
Menu Items will lose border radius when they have no padding or 0 padding (p-0).
here's and example: https://play.tailwindcss.com/2nJyj7OqfS

@MentalGear
Copy link
Contributor Author

MentalGear commented Nov 13, 2022

First off, thanks for the super quick response @saadeghi !

I think there's some misunderstanding however regarding the expected behaviour, and therefore I'd like to reopen this issue.

In my opinion, --rounded-btn should be applied independently of whether a p- class is present on the (preceding) element or not, so that it is self-dependent. For example, there are situations where you might have padding applied not on the menu itself but on an overarching element.

Also, in the new version, if you switch themes in the docs, none of the menu items have a rounded border applied anymore, whereas previously, depending on the theme's --rounded-btn setting, the menu item had roundings aligned with the btn class. Now, for all themes all menu items are 0px radius - which seems odd.

I guess it also boils down whether you consider the menu's a links to be 'buttons'. In my opinion it would make sense, since a normal a link can also have a btn class.

What do you think?

PS: Regarding the testing: It's great that accessibility is a priority and has proper tests! Yet, I think visual regression testing would also be highly useful and I shall add a new proposal for it.

@saadeghi
Copy link
Owner

I don't think the padding of list and the border radius of the list item should be independent from each other. That might sound a weird statement but here's the reason:

  • If our list has padding a list item with no border radius that wouldn't look good and wouldn't match the design language because it would be a sharp rectangle inside another:

2

  • If the list has 0 padding but the list item has border radius, the list item will be rectangle with border radius sticked to the border of list. That would not look good either:

1

So instead of putting everything on the shoulders of the developer, there's a style that detects if the list has padding or not and applies border radius for the list item only if there's no padding. That would make the menu look better and in sync with the overall design language.

As you can see, there are also specific rules about the first and last items and it gives them border radius and start and end if there's border radius for the list.

Also, each built-in daisyUI theme can have its own value for --rounded-btn so each theme can have a different look and feel. For example, --rounded-btn is rounder in valentine theme and it has no radius for black theme. So, as expected when we use black theme there would be no border radius at all.

@MentalGear
Copy link
Contributor Author

I can understand your reasoning and I much appreciate the effort to reduce cognitive load.

I'm aware that --rounded-btn is settable per theme, yet you should be aware that the guide's side menu items now - no matter the theme - have all no radius. Previously, themes like "cupcake" which, uses rounded border for buttons, had the same rounding in the menu items, which made it feel much more in "sync" with the overall theme. Anyone using DaisyUI themes will also experience having their drawer / menu items 0px radius transformed. Maybe we need to add an exception here somewhere. I don't think it is what you intended.

@saadeghi
Copy link
Owner

What is your proposition?
Can you please give me a piece of HTML and tell me about your "expectation vs. reality"? because I'm not sure I understand the issue here 😅

@MentalGear
Copy link
Contributor Author

I agree with your behaviour for the automated menu li rounding.

The only thing I wanted to note is that, in a full-page menu within a drawer, like the guide previously had rounded li items for themes with --rounded-btn > 0 whereas now all li items are the same, no matter the theme. Personally, I think the previous appearance was more in line for a global theme style.

Therefore I wondered, if it isn't better, at least for menus in drawers, to keep the rounding for menu items.

Screenshot 2022-11-15 at 16 09 54

saadeghi added a commit that referenced this issue Nov 16, 2022
@saadeghi
Copy link
Owner

Yes you're right. The sidebar menu is fixed now.

@shansurat
Copy link

It's working for the sidebar now but not for the navbar ul>li elements.

inorganik pushed a commit to inorganik/daisyui that referenced this issue Feb 7, 2023
inorganik pushed a commit to inorganik/daisyui that referenced this issue Feb 7, 2023
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

No branches or pull requests

3 participants