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

569-new-section-carousel-first-full-layout #570

Merged
merged 5 commits into from
Dec 12, 2023
+85 −132
Merged

569-new-section-carousel-first-full-layout #570

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
62367fd
updated for new section
rgravitvl Dec 11, 2023
ebb3cd3
fixed lint issues
rgravitvl Dec 11, 2023
4f09fd3
updated carousel to handle multiple
rgravitvl Dec 11, 2023
d4d9a2d
fixed lint issues
rgravitvl Dec 11, 2023
d4d1d1d
updated converter for full layout
rgravitvl Dec 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions tools/actions/convert/test/fixtures/en-converted.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ <h2>Together, we see a way</h2>
</div>
</div>
<hr>
<hr>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check these 2 sections? The homepage is not using any carousel, so it should not be impacted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As we separated section of first full layout, home page is also got additional

<div class="cards articlecard">
<div>
<div>
Expand Down
2 changes: 2 additions & 0 deletions tools/actions/convert/test/fixtures/product-opco-home-converted.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,14 @@ <h2>The world leader in microscopes and scientific instruments</h2>
</div>
</div>
</div>
<hr>
<h2>Product Categories</h2>
<div class="product-category-list opco-home">
<div>
<div></div>
</div>
</div>
<hr>
rgravitvl marked this conversation as resolved.
Show resolved Hide resolved
<div class="columns">
<div>
<div>
Expand Down
1 change: 1 addition & 0 deletions tools/actions/convert/test/fixtures/product1-converted.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ <h2>This is a tab with just the hero image</h2>
</div>
</div>
</div>
<hr>
<h2>Categories</h2>
<div class="product-category-list">
<div>
Expand Down
2 changes: 1 addition & 1 deletion tools/importer/transformers/carousel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ const createCarousel = (main, document) => {
});
cells.push(...carousels);
const block = WebImporter.DOMUtils.createTable(cells, document);
carousel.append(block);
carousel.append(block, document.createElement('hr'));
}
};

Expand Down
1 change: 1 addition & 0 deletions tools/importer/transformers/fullLayoutSection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const createFullLayoutSection = (main, document) => {
const div = e.querySelector('div');
const style = div.getAttribute('class');
if (style) {
if (i === 0 && e.parentNode.previousElementSibling) e.prepend(document.createElement('hr'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

just wondering why we need this in the section? I thought the hr tag can be added to individual blocks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This would needed to separate fulllayout from other above components. I can explain over the call. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

@duynguyen we (Ravi & I) just discussed we should have the sections controlled by the containers and not the blocks. Adding

before / after a block is risky, might work on one place but can have side effects on other places the same block is used.

const cells = [['Section Metadata'], ['style', style]];
const table = WebImporter.DOMUtils.createTable(cells, document);
e.after(table);
Expand Down