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

Fix Slider content is underlined in mobile & Slider button color #453

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

iRohitSingh
Copy link
Member

Fix #452

Screenshot 2024-12-19 at 6 01 24 PM

@iRohitSingh iRohitSingh added the bug Something isn't working label Dec 19, 2024
@davisagli
Copy link
Member

davisagli commented Dec 19, 2024

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 a tag is inside the .teaser-item instead of the other way around: https://github.com/kitconcept/volto-light-theme/blob/main/packages/volto-light-theme/src/theme/_bgcolor-blocks-layout.scss#L27

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?

@iRohitSingh
Copy link
Member Author

iRohitSingh commented Dec 20, 2024

https://github.com/kitconcept/volto-light-theme/blob/main/packages/volto-light-theme/src/theme/_bgcolor-blocks-layout.scss#L27

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.

@sneridagh
Copy link
Member

sneridagh commented Dec 20, 2024

@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.

@davisagli
Copy link
Member

@davisagli
Copy link
Member

@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.

@sneridagh
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slider content is underlined in mobile & Slider button color is wrong
3 participants