Indent nested taxon menues and highlight the selected taxons. #2316
Conversation
|
Hello! It would be great if you could explain better which problem this PR will solve, and maybe post some screenshot of the result so it is immediately clear to everyone. Thanks for your contribution! |
Without this fix, the taxon menu looks like this
With this fix, the taxon menu looks like this
|
| a { | ||
| font-size: $main_navigation_font_size | ||
| } | ||
| } | ||
| .current { | ||
| font-weight: bold; | ||
| } |
There was a problem hiding this comment.
I think this code can be simplified in this way:
ul.taxons-list {
ul {
margin-left: 1em;
}
li {
font-size: $main_navigation_font_size;
font-weight: normal;
&.current {
font-weight: bold;
}
}
}There was a problem hiding this comment.
I agree it can be simplified a little.
However &.current is a too specific selector.
There was a problem hiding this comment.
I think it works with &.current since it applies to ul.taxons-list li, which is exactly elements that need this style.
There was a problem hiding this comment.
@kennyadsl I think @bofrede meant that the selector is too strong in terms of "Do not over specify css selectors" rule of thumb. It would be hard to override this if we over specify selectors. 90% of !important usage is because CSS authors uses too specific css selectors.
There was a problem hiding this comment.
@tvdeyen thanks for your feedback. Yes, it's probably better to have it less specific.
| a { | ||
| font-size: $main_navigation_font_size | ||
| } | ||
| } | ||
| .current { | ||
| font-weight: bold; | ||
| } |
There was a problem hiding this comment.
@kennyadsl I think @bofrede meant that the selector is too strong in terms of "Do not over specify css selectors" rule of thumb. It would be hard to override this if we over specify selectors. 90% of !important usage is because CSS authors uses too specific css selectors.
kennyadsl
dismissed
their stale review
It's bettter to squash commits into a single one
|
Now simplified and squashed as per @kennyadsl's request. |
|
Thanks! |
|
Sorry that this took so long to be merged. Please feel free to ping us, if an approved PR takes too long to be merged. |
To see this in action you need:
config.max_level_in_taxons_menu = 3inconfig/initializers/spree.rb