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.

Sign up for GitHub

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

Jump to bottom

Upgrade and adapt the theme tovolto-slider-block 6.0.0 add-on #288

Merged
merged 11 commits into from
Dec 22, 2023
Merged

Upgrade and adapt the theme tovolto-slider-block 6.0.0 add-on #288

merged 11 commits into from
Dec 22, 2023

Conversation

sneridagh
Copy link
Member

No description provided.

@sneridagh sneridagh changed the title Adapt newslider (6.0.0 alpha.0) in light theme Adapt new slider (6.0.0 alpha.0) in light theme Dec 5, 2023
@tisto
Copy link
Member

tisto commented Dec 5, 2023

@sneridagh does that mean we have a hard dependency and the old slider will stop working? I'd be ok with that. However, if that is the case, we should start some kind of compatibility matrix.

@sneridagh
Copy link
Member Author

No, it won't stop working.
The change is in the CSS classes of the main slider elements (not the inner ones). Which is rarely styled, beside margins/paddings.
The good news is that the structure is the same, the classnames are different but they are mapped 1:1 with the old ones. I already documented the mapping in the add-on (see README there).
Only a small adaptation (class bulk rename and CSS check is required. I did the adaptation literally in 15m.
No worries, all is under control.

@ichim-david
Copy link
Collaborator

@sneridagh I've tested your branch and the slider is indeed a drop-in replacement.
There are a couple of enhancements that I would like but I will add them in the add-on as an issue.

@sneridagh
Merge branch 'main' into adaptnewslider6.0.0-alpha.0-inlighttheme
7b143f6 
* main:
  Un-pin eslint-plugin-jsx-a11y version (#299)
  Release 2.1.0
  Update contentMetadataTagsImageField settings to use preview_image field(#274) (#284)
  Add Brazilian Portuguese translation (#295)
  Fixes #279 load custom fonts from a separate file and allow setting of cms font instead of hardcoding to Metropolis  (#280)
  Fix Image View forces in image content type (#292)
  Also clean up Navigation PropTypes (#285)
@sneridagh
Last fixes with alpha.1
48fac04
@sneridagh sneridagh changed the title Adapt new slider (6.0.0 alpha.0) in light theme Adapt new slider (6.0.0 alpha.1) in light theme Dec 15, 2023
@sneridagh
Copy link
Member Author

@tisto @ichim-david I think it's ready.

@iRohitSingh could you please take a look?

@sneridagh
Copy link
Member Author

@davisagli @danalvrz This is ready, please take a final look.

@iRohitSingh
Copy link
Member

@sneridagh Functionalities by it is working perfectly fine but style is totally missing.

Screenshot 2023-12-15 at 7 39 46 PM Screenshot 2023-12-15 at 7 38 44 PM Screenshot 2023-12-15 at 7 36 39 PM Screenshot 2023-12-15 at 7 35 36 PM Screenshot 2023-12-15 at 7 46 12 PM

danalvrz
danalvrz approved these changes Dec 15, 2023
View reviewed changes
Copy link
Contributor

@danalvrz danalvrz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it works great!

@sneridagh
Copy link
Member Author

@iRohitSingh I think you tested the wrong version:

image

Where are the dots coming from?

sneridagh and others added 2 commits December 18, 2023 10:53
@sneridagh @ichim-david
Update src/theme/blocks/_slider.scss
54c6653 
Co-authored-by: ichim-david <ichim.david@gmail.com>
@sneridagh
Update versions
65b7259
@sneridagh sneridagh changed the title Adapt new slider (6.0.0 alpha.1) in light theme Adapt new slider (6.0.0) in light theme Dec 18, 2023
@sneridagh
Copy link
Member Author

sneridagh commented Dec 18, 2023 •
edited
Loading

@ichim-david I reverted your suggestion, because thinking it twice, that padding is fixed, and not dependent of the vertical spacing variable. It's exactly that one for a reason, and it's pixel perfect according with the mocks. It could be 43px, but it's 40px, by chance. Meaning, I would not want it to be changed if I change the spacing var.

@iRohitSingh iRohitSingh mentioned this pull request Dec 21, 2023
Merged
@sneridagh sneridagh changed the title Adapt new slider (6.0.0) in light theme Upgrade and adapt the theme tovolto-slider-block 6.0.0 add-on Dec 22, 2023
@sneridagh sneridagh merged commit 15cda79 into main Dec 22, 2023
7 checks passed
@sneridagh sneridagh deleted the adaptnewslider6.0.0-alpha.0-inlighttheme branch December 22, 2023 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ichim-david ichim-david ichim-david left review comments

@danalvrz danalvrz danalvrz approved these changes

@davisagli davisagli Awaiting requested review from davisagli

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sneridagh @tisto @ichim-david @iRohitSingh @danalvrz