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

Hotfix: Headline / Blockquote Text Alignment Regression Fix #1794

Merged
merged 7 commits into from
Mar 26, 2020

Conversation

sghoweri
Copy link
Contributor

@sghoweri sghoweri commented Mar 25, 2020

Jira

http://vjira2:8080/browse/BDS-2074

Summary

Addresses a regression in last week's v2.20.0 release in which the new Numbered Bullet feature added to Headline component inadvertently caused the component's text alignment to not work as expected in certain situations.

Details

  1. While Headline component's horizontal alignment (via the component's align prop) continues to work as expected, there were unfortunate side-effects from the globally applied display: flex; CSS update that caused text alignment to stop working in the Blockquote component + when the u-bolt-text-align-X utility class was used.

This PR works around that issue by only adding the display: flex; styles + related c-bolt-headline__text inner text wrapper in the specific use case that required both of those updates in the first place -- when a Headline is displaying the new Numbered Bullet option.

  1. This PR also ensures that the c-bolt-headline__text wrapper continues to be conditionally added when the quoted version of Headline is used (which the original updates in v2.20.0 addressed)

  2. This PR also fixes a separate issue related to this regression with Blockquote quote styles not rendering as expected once the page's JavaScript kicks in

  3. Finally, this PR cherry picks the one critical update from Hotfix: Fix for Composer Install Issues on Travis #1793 to ensure CI builds continue to work as expected.

How to test

  1. Confirm new text alignment-related VRT snapshots added for Blockquote look correct
  2. Compare before and after links for Headline, Blockquote, and the referenced utility class instance to confirm these updates fix the visual regressions.

Headline demos are visually similar in v2.20.0 compared to this PR

  • Confirm that the Headline is visually as good or better as it was in the v2.20.0 release
  • Confirm that the quotes that display when the Headline's quoted option is used show up as expected
  • Confirm that the Right Icon in Headline is still inline when text wraps on smaller screens

Blockquote text alignment broken in v2.20.0 is now fixed

  • Confirm that the Headline's text alignment usage within Blockquote is fixed in this PR when compared to the previous v2.20.0 release

The broken Utility class alignment for "Add Mission" in v2.20.0 is now fixed

  • Confirm that the Headline component's text alignment is now fixed with this updates from this PR.

@sghoweri sghoweri added this to the v2.20.1 milestone Mar 25, 2020
@sghoweri sghoweri changed the base branch from master to release/2.x March 25, 2020 21:10
Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

@sghoweri the main regression is fixed 👍I just want to be sure that we're not causing any new ones.

  1. I left an inline comment about the space between position "before" icons and text.
  2. Headlines icon now display the icons slightly higher than before (because icon is inline-flex now).

@@ -119,10 +120,10 @@

{% include "@bolt/link.twig" with linkVars only %}
{%- else -%}
<span class="c-bolt-headline__text">{% spaceless %}
{% if iconAfter or quoted %}<span class="c-bolt-headline__text">{% endif %}{% spaceless %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not outputting this <span>^ seems to be causing the arrow icon in the Numbered Headline example to display too close to the text:
image

The missing space is just whitespace that no longer shows because it's in an inline-flex container. You could fix by always outputting a plain span, or by always outputting <span class="c-bolt-headline__text"> and adding a modifier when you want to make this span inline-block.

Copy link
Collaborator

@danielamorse danielamorse Mar 26, 2020

Choose a reason for hiding this comment

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

Update: I now see that we cannot "always output a plain span". This actually causes the Blockquote quotes to disappear.

The issue seems to be that whitespace is going to be treated differently when it's block vs flex and always adding a span is not a solution. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danielamorse one idea that I had was to try removing the extra whitespace when the UI renders using inline-block + re-add manually for more consistent spacing. WIll take a look!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Edit: the issue here might actually just physical whitespace not being removed from the inner text content.... HMMM

…on within the Headline component; update PL demo to include demoing icons at different Headline sizes
@sghoweri
Copy link
Contributor Author

@danielamorse this should be ready for you to take another look 👍

Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

@sghoweri I retested cross-browser and the space around icons is nice and consistent 👌

The regressions are fixed, ship it!

@sghoweri
Copy link
Contributor Author

Thanks @danielamorse!

@sghoweri sghoweri merged commit 347ac66 into release/2.x Mar 26, 2020
@sghoweri
Copy link
Contributor Author

PR was released with v2.20.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants