Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

Shrink brand before wrap #2889

Merged
merged 18 commits into from
Jan 16, 2020
Merged

Shrink brand before wrap #2889

merged 18 commits into from
Jan 16, 2020

Conversation

AlistairGempf
Copy link
Contributor

@AlistairGempf AlistairGempf commented Jan 9, 2020

Resolves #NUMBER

Overall change: Shrinks the brand svg before the script link wraps onto a new line (except on IE which an IE bug has prevented from working). Previously, the script link would wrap onto a new line before the brand svg started shrinking. This PR makes the brand svg shrink as much as possible before putting the script link onto a new line.

Code changes:

  • Use flex-basis of the svg minimum width
  • Use flex-shrink and flex-grow to allow the svg to change width
  • Keep the -ms-flex-preferred-size at the max width to prevent this bug

  • I have assigned myself to this PR and the corresponding issues
  • Automated jest tests added (for new features) or updated (for existing features)
  • This PR requires manual testing

@AlistairGempf AlistairGempf added the ws-home Tasks for the WS Home Team label Jan 9, 2020
@AlistairGempf AlistairGempf self-assigned this Jan 9, 2020
@AlistairGempf
Copy link
Contributor Author

Blocked on #2887

@AlistairGempf
Copy link
Contributor Author

Chromatic is showing
image on IE. Not sure why it would do that or how to investigate on IE.

@AlistairGempf
Copy link
Contributor Author

The problem is that IE11 calculates the flex spacing based on the element's flex-basis size, then adds that much spacing between the elements, even if the element is larger than its flex-basis.

In order to have the shrink happen before the wrap, the flex basis needs to be the min-width. This leads IE11 to add the spacing required for the min-width even when the svg is at max-width, pushing the script link off the edge of the window.

@AlistairGempf
Copy link
Contributor Author

AlistairGempf commented Jan 9, 2020

One possible solution is to use the css -ms-flex-preferred-size: ${maxWidth / 16}rem to override the flex-basis set in the flex property only on IE browsers.

This would prevent the shrinking happening before the wrap, but IE must almost entirely be used on desktop so this loss of functionality will not impact many people and it would display the script link reasonably at all widths.

Copy link
Contributor

@hotinglok hotinglok left a comment

Choose a reason for hiding this comment

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

LGTM, tested on storybook and IE on browserstack and the functionality seems to be working fine, nice work!

@@ -71,6 +71,8 @@ const brandWidth = (minWidth, maxWidth) => `
width: 100%;
max-width: ${maxWidth / 16}rem;
min-width: ${minWidth / 16}rem;
flex: 1 1 ${minWidth / 16}rem;
-ms-flex-preferred-size: ${maxWidth / 16}rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

The update to flex works well on supported browsers.
For -ms-flex-preferred-size, I looked on Browserstack with IE11 and removed this style and did not see any change in behaviour. Am I missing something here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

image

This is what I see with -ms-flex-preferred-size turned off on IE11 - the script switch is off the edge of the screen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like I had an issue with the changes in the browserstack dev tools not applying correctly. When I made changes locally I see the same as your screenshot.
So, I'm happy with the change made!

Copy link
Contributor

@sareh sareh 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. I've reviewed in on latest Chrome & Firefox as well as IE11.

@paruchurisilpa paruchurisilpa self-assigned this Jan 14, 2020
@paruchurisilpa
Copy link
Contributor

Looks good to me..

@tochwill
Copy link
Contributor

@paruchurisilpa is this ok to merge then?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ws-home Tasks for the WS Home Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants