-
Notifications
You must be signed in to change notification settings - Fork 9
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
Fix Slider content is underlined in mobile & Slider button color #453
base: main
Are you sure you want to change the base?
Conversation
There is already a rule here which is supposed to prevent underlining links inside a teaser, but it doesn't work in this case because the I think we should fix that rule OR fix the markup of the teaser-slider block, instead of adding an override. @sneridagh could you take a look? |
I also saw this code and get confused, since the markup is different I written the new css for that. @sneridagh Here is the code for slider. We are already customised it. If you want I can change the template put the markup other way around. https://github.com/kitconcept/volto-light-theme/blob/main/packages/volto-light-theme/src/components/Blocks/Slider/DefaultBody.jsx#L99C25-L99C37 Also this css is little bit problamatic. If you put anything inside a with not direct children class it will apply the text-decoration and the only options is to override it. But most of time this is what anybody will do. A good solution only put text-decoration when it only contains text node no direct block level element. In this way it guranteed that a only contains presentational element like icon and text. Or provide a class-name like for this interactive element. Like we do for external link. |
@iRohitSingh I think it's better we unify markup instead of adding a new CSS exception. Since we are already customizing the slider, let's do that, and remove the CSS exception. Could you please make this amendment? Regarding your suggestion, I think it would be a good addition, maybe you can open a PR with it? However, I don't know if doing it would be breaking other's theming. We will have to investigate it. |
@sneridagh Isn't linking block elements an anti-pattern from an a11y perspective? https://css-tricks.com/block-links-are-a-pain-and-maybe-just-a-bad-idea/ |
@sneridagh @iRohitSingh Unfortunately I think it's probably too big of a breaking change to change the markup in volto-slider-block. However, looking again at the rule https://github.com/kitconcept/volto-light-theme/blob/main/packages/volto-light-theme/src/theme/_bgcolor-blocks-layout.scss#L27, I think we could just add the "item" class to this link in volto-slider-block. |
@davisagli I vaguely remember that we did it this way because of something. I can't remember what exactly, maybe DLR asked explicitly for having the link for the whole block rather than for the inner elements? We are still in alpha, we could still break things. We will have to fix/improve it at some point, specially when this already bite us a couple of times already. For now, I'll merge the fix for .com, then when we have time, let's decide on this one after Christmas. |
Fix #452