Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Content] Add diogogpinto-filament-themes-full-guide.md content and respective art #587
base: main
Are you sure you want to change the base?
[Content] Add diogogpinto-filament-themes-full-guide.md content and respective art #587
Changes from 1 commit
cd73873
af8f36d
b2c271d
6b5a38e
72f2c3d
261df8e
96cd409
2270af5
ff71373
836f4a0
5a52631
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I actually think this isn't necessarily always going to work.
I believe that, specifically referencing the bottom margin, we won't reliably be able to override the CSS styling. This is because there is already a
mb-2
class in the class string, so when our styles get applied via.fi-breadcrumbs
,mb-2
and ourmb-4
will (theoretically) have the same specificity. If that is the case, whichever class is defined further down in the cascade with take precedence.I might be wrong here, but regardless it may be worth changing this example from an "overriding" example to an "adding" example. Something like adding a top padding, border, etc.
@zepfietje (or anyone from Core), let me know if I'm completely incorrect here.
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.
@alexandersix just wondering, given that the custom classes are always defined after TW classes, wouldn't they always take precedence? Or is there a case where that may not be true?
I may be missing something
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.
See above comment, but I believe the order in which a class string is laid out has no bearing on specificity or the cascade, so having multiple classes of the same specificity may lead to unreliable outputs based on where the Tailwind classes are defined in relation to one another.
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.
Yup, totally depends on specificity.
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.
@zepfietje @alexandersix
I'm probably missing something, given that both TW classes and custom classes created with @apply are technically single class selectors, shouldn’t they all have equal specificity?
I clearly made a mistake in the text, as it isn't order of declaration that matters, but the build process that builds custom classes after the utility classes.
Hope to get some clarity, thank you in advance!
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.
@zepfietje hey Zep, sorry for pinging you on this, but just so I can ship the changes, in the text I'm considering saying something like:
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.
Do you think this makes sense?
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.
It is confusing as to why you have the same class twice, with one being more specific due to the
span
selector. Could you combine these two together?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.
@alexandersix I believe in this case breadcrumbs active item is wrapped in a
<span>
tag, the others on a<a>
tag.Maybe something like the following would be better?
Will have to try it out.
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.
We really should not have this section in my opinion as this approach should basically never be used.
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.
I'm on the fence about this one. On the one hand, I completely agree that we almost never want this approach to be used. However, on the other hand, by having this here, we're specifically calling out the fact that this is the wrong approach, dissuading people from using it.
I guess it comes down to whether or not this is going to prompt more people to remember that this is an option and use it or if it's going to prevent people from using it because it's explicitly stated not to.
I think I come down more on the side of leave it in to ensure that people know it's the wrong way to do things. My hunch is that the people reading this article are (in general) going to be deep enough in Filament to know that overriding views is an option in Laravel 🤷🏻
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.
I can see both points!
I inserted because sometimes you may want to change specific functionality around whole components that may not necessarily break functionality on updates. My case was the topbar, but later I disabled it and used render hooks. But that approach gave me some headaches around mobile and had to create a whole new mobile trigger for the sidebar - if I had to do it all over again, I would publish the file and monitor each release for breaking changes.