Skip to content

Conversation

@saashapina
Copy link
Collaborator

@saashapina saashapina commented Apr 22, 2025

Summary of changes

  • adds horizontal rule
  • adds horizontal rule tests

Relevant Links

Test URLs:

Checklist

  • This PR has visual changes, and has been reviewed by a designer.
  • This PR has code changes, and our linters still pass.
  • This PR has new code, so new tests were added or updated, and they pass.

Validation

  1. Make sure all PR checks have passed.
  2. Pull down all related branches.
  3. Verify horizontal rule aligns with mockups
  4. Verify all tests are passing

Browser Testing

We should aim to support the latest version of the listed browsers. For older versions or other browsers not on the list, content should be accessible, even if it doesn't completely match the designs.

Developers should test as they work in the browsers available on their machines. If they have access to other devices to test other browser/OS combinations, they should do that when possible.

Windows

  • Firefox
  • Chrome
  • Edge

MacOS

  • Firefox
  • Chrome
  • Safari
  • Edge

Android

  • Firefox
  • Chrome
  • Edge

iOS

  • Safari

@aem-code-sync
Copy link

aem-code-sync bot commented Apr 22, 2025

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

@aem-code-sync
Copy link

aem-code-sync bot commented Apr 22, 2025

Page Scores Audits Google
📱 /pattern-library/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
🖥️ /pattern-library/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

Copy link
Collaborator

@catheraaine catheraaine left a comment

Choose a reason for hiding this comment

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

Okay I like how this is set up.

The ticket says we need to solve for both full-width and contained-width versions.

I think what I would like for us to do is make the contained-width version the default, so we can remove the 150vw css from there. This would apply for the majority of use-cases, say, blog post content.

The full-width version is used pretty rarely, but I like your solution and I want to keep it. Let's move those styles to a .horizontal-rule--full modifier class, and if we need to use them in another component we will solve that later (potentially by applying the class in the document in those few cases? this would prevent the content entry team from using the wrong one without design approval).

Should be a small change then we can merge.

@saashapina
Copy link
Collaborator Author

The full-width version is used pretty rarely, but I like your solution and I want to keep it. Let's move those styles to a .horizontal-rule--full modifier class, and if we need to use them in another component we will solve that later (potentially by applying the class in the document in those few cases? this would prevent the content entry team from using the wrong one without design approval).

Should be a small change then we can merge.

Updated! I added the <hr-fw> tag as an identifier for full-width horizontal rules

Copy link
Collaborator

@arnest00 arnest00 left a comment

Choose a reason for hiding this comment

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

Great work on this!!

@arnest00 arnest00 self-requested a review April 25, 2025 19:52
* Converts '<hr>' and '<hr fw>' text nodes into horizontal rule elements
* @param {Element} element container element
*/
function decorateHorizontalRules(element) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

🏆, by the way.

@arnest00 arnest00 dismissed catheraaine’s stale review April 25, 2025 20:15

Changes requested were implemented!

@saashapina saashapina merged commit 36e6376 into main Apr 25, 2025
5 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants