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

Responsive menu components incorrect class name / not working properly with prefix #578

Closed
camslice opened this issue Mar 11, 2022 · 18 comments
Assignees
Labels

Comments

@camslice
Copy link
Contributor

The combination of menu md:menu-horizontal does not seem to work. However menu md:horizontal does work.

Daisy UI v2.6.4
Tailwind v3.0.23

@saadeghi saadeghi added the menu label Mar 11, 2022
@saadeghi
Copy link
Owner

It works: https://play.tailwindcss.com/SZeA5fg4jk?size=774x279
Please share your code so I can reproduce the issue

@camslice
Copy link
Contributor Author

camslice commented Mar 14, 2022

Hi @saadeghi , thanks for the playground link. I really should have used this when I first reported the issue!

It does not seem to work with a prefix: https://play.tailwindcss.com/sgF3ciI8gd?size=942x279

I've updated the Issue title accordingly. Let me know if I can help in any other way. Thanks

@camslice camslice changed the title Responsive menu components incorrect class name Responsive menu components incorrect class name / not working properly with prefix Mar 14, 2022
@camslice
Copy link
Contributor Author

FYI @saadeghi it was actually a colleague of mine @vnphanquang who implemented the prefix feature. We're using it on a big project to incrementally replace an existing library. Should really help with the adoption of Daisy UI.

@vnphanquang
Copy link
Contributor

I will take a look at this guys

@camslice
Copy link
Contributor Author

Thanks @vnphanquang ! 👋

@vnphanquang
Copy link
Contributor

Gentlemen please take a look at the PR here. The fix is at this commit

Reason is because i was making direct mutation to the import, and since the tailwind plugin might be ran multiple times, the prefix ends up being duplicated for the utilities (into something like pre-pre-pre-pre-classname). My bad I didn't catch this earlier.

@saadeghi There are two other commits in this PR, one is my proposal to add an output config option for debugging purposes (a filepath to output all daisy styles to).

The last commit is a section in the CONTRIBUTING guideline as a reminder to devs to add $$ in pre blocks to render the dynamic prefix since I've seen some people missed this recently

Let me know. Thanks all!

@saadeghi
Copy link
Owner

@vnphanquang thanks my friend 👏
I'm just not sure about the new output config. If it's just for debugging this issue, should it really be in the package?

@vnphanquang
Copy link
Contributor

vnphanquang commented Mar 16, 2022

@saadeghi I was considering the same thing.

I think this might be helpful to some advanced use cases like the project @camslice is working on where users want to see the output from daisy without having to read our source code.

Problems like the one in this issue can be detected easily from the end user with this log too.

Your call really. Let me know.

@vnphanquang
Copy link
Contributor

Ah one other thing, I'm not sure if the imports of fs and path here will break tailwind play again. Will check this in 2 hours sorry.

@saadeghi
Copy link
Owner

saadeghi commented Mar 16, 2022

I can't think of any other use case for output config. And as you mentioned, it might cause problems on strict environments like Tailwind Play or maybe different build tools.
Maybe we can add test scripts later on to make sure class names are generated correctly but for now, I don't think the output config would be helpful for the majority of developers.

@vnphanquang
Copy link
Contributor

Agreed. Let me remove that commit.

@vnphanquang
Copy link
Contributor

Commit removed, pr good to go now @saadeghi

@camslice
Copy link
Contributor Author

Sounds fine to me @saadeghi . Better to avoid potential issues in strict environments. Test scripts sound like a good feature for the future. Thanks for the fix @vnphanquang !

@camslice
Copy link
Contributor Author

camslice commented Mar 16, 2022

Just a quick note here @saadeghi @vnphanquang. When inspecting the CSS output, I am noticing these classes:

.menu {
  display: flex;
  flex-direction: column;
  overflow: hidden;
}
.menu.horizontal {
  display: inline-flex;
  flex-direction: row;
}
.menu.horizontal :where(li) {
  flex-direction: row;
}

As per the original bug report, the horizontal class actually works when combined with menu. However there is no "horizontal" component as such.

You can see it here, just inspect the CSS in the browser dev tools: https://play.tailwindcss.com/Vac3ty7olI?size=774x279

You will notice this:
image

Another example without the responsive prefix: https://play.tailwindcss.com/NVcXW6Ddkw?size=774x279
image

Does this look like a bug to you?

@saadeghi
Copy link
Owner

@camslice how about menu-horizontal? horizontal has been replaced by menu-horizontal but I didn't remove it yet because of backward compatibility.

@saadeghi
Copy link
Owner

IMG_20220316_194010.jpg

@camslice
Copy link
Contributor Author

@saadeghi ah that explains it. It's there for backwards compatibility. Yep menu-horizontal works fine. I was just confused by the presence of the horizontal class.

@saadeghi
Copy link
Owner

Fixed in 2.9.0 thanks to @vnphanquang
https://play.tailwindcss.com/A5wRpZAFt5?size=950x279&file=config

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

No branches or pull requests

3 participants