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

SHS 5130 Use <hr> when there is no color band text #1398

Merged
merged 4 commits into from
Nov 21, 2023

Conversation

mndonx
Copy link
Contributor

@mndonx mndonx commented Nov 18, 2023

READY FOR REVIEW

Summary

This PR checks the Color Band component for text, and, if there is no text, prints an


tag instead of an empty heading tag.

Urgency

medium

Steps to Test

  1. Edit a page
  2. Add a new page component, and choose Color Band
  3. Do not add text to the text field and save
  4. Ensure that if the color band has no text, an <hr> tag prints inside the color band.
  5. Ensure that color bands with text still print the same as before.

PR Checklist

@mndonx mndonx requested a review from cienvaras November 18, 2023 02:11
@cienvaras cienvaras changed the base branch from 10.0.3-release to fk-stnfd-sprint-39 November 20, 2023 01:27
Copy link
Collaborator

@cienvaras cienvaras left a comment

Choose a reason for hiding this comment

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

@mndonx Please see the code comments. Also, please check styles. In Chrome, the hr has an inset border. Make sure that the hr styles match the empty h2.

{% if color_band_text %}
<h2 class="hb-color-band--text">{{ color_band_text }}</h2>
{% else %}
{{ '<hr class="hb-color-band--text" />' }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just add the html tag directly?

<hr class="hb-color-band--text" />

I might be missing something, but I see no need to print it as a twig string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea why I did this!

@mndonx mndonx requested a review from cienvaras November 20, 2023 16:32
@mndonx
Copy link
Contributor Author

mndonx commented Nov 20, 2023

Thanks @cienvaras, ready for re-review.

@@ -73,4 +73,8 @@
padding: hb-calculate-rems(20px);
}
}

hr {
border: 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@mndonx We're almost there, just a little extra thing to fix. The top and bottom margin is different in the hr. There are not explicit rules in the theme for that, but the browser is adding some default styles. I checked in multiple browsers and it's consistent, so it should be fixed adding this to the hr styles:

margin-block-start: 0.83em;
margin-block-end: 0.83em;
font-size: 3.6rem;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cienvaras Good eye, thank you! I also found another instance where the logic needed to be applied (in the original pattern.) Let me know if you spot anything else. Thank you!

@mndonx mndonx requested a review from cienvaras November 21, 2023 06:39
Copy link
Collaborator

@cienvaras cienvaras left a comment

Choose a reason for hiding this comment

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

Works great, thanks 🚀

@cienvaras cienvaras merged commit 4eb18bb into fk-stnfd-sprint-39 Nov 21, 2023
@cienvaras cienvaras deleted the SHS-5130-hr-tag-color-band branch November 21, 2023 17:45
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.

None yet

2 participants