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

fix(core-price-lockup): fixing horizontal alignment issue #836

Merged
merged 3 commits into from Dec 10, 2018

Conversation

nicmak
Copy link
Contributor

@nicmak nicmak commented Nov 26, 2018

Related issues

#825

Description

  • wrapped a div with inline-block and alignment left to account FlexGrid Row horizontal alignment changes

Checklist before submitting pull request

  • New code is unit tested
  • For code changes, run yarn prepr locally
    • make sure visual and accessibility tests pass

wrapped a div with inline-block and alignment left to account FlexGrid Row horizontal alignment changes
@TDSBot
Copy link

TDSBot commented Nov 26, 2018

Changes:
 - @tds/core-price-lockup: 1.0.1 => 1.0.2


🤔 If this is not what you expected, ensure that your commit messages follow the Conventional Commits specification (https://conventionalcommits.org) and try again.

🚀 Please paste the entire output of this task into the body of your Pull Request so that a maintainer can verify before merging/publishing.

@nicmak nicmak requested a review from jraff November 26, 2018 19:51
Copy link
Contributor

@theetrain theetrain left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix! Please address my two comments and I'll get this merged soon.

@@ -1,6 +1,11 @@
@import '~@tds/core-responsive/responsive';
@import '../../shared/scss/typography';

.wrapperAlignment {
text-align: left;
display: inline-block;
Copy link
Contributor

Choose a reason for hiding this comment

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

This fix can be achieved without display: inline-block

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If i remove inline-block (this is when I include the code inside box component), horizontal alignment does not work from FlexGrid.Row.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we release this using inline-block then it will be a breaking change because it would cause subsequent inline elements to appear inline, for example:

image

<PriceLockup
  size="medium"
  topText="Starting at"
  price="25"
  rateText="/month"
  bottomText="$68 /month after 3 months"
  signDirection="left"
/>
<ButtonLink href="#">Click me</ButtonLink>

We have backlogged a patch to have Price Lockup have a max width, that way the hairline divider's width won't need to be controlled by the developer via FlexGrid: https://telusdigital.atlassian.net/browse/TDS-1042

packages/PriceLockup/PriceLockup.jsx Outdated Show resolved Hide resolved
@theetrain
Copy link
Contributor

Thanks for the update, please address the following:

  • Remove inline-block (see comment above)
  • Update Jest snapshot and e2e screenshot for the build to pass

removing inline-block and updating screenshots and tests
@TDSBot
Copy link

TDSBot commented Dec 4, 2018

Changes:
 - @tds/core-breadcrumbs: 1.0.0-dev => 1.0.0
 - @tds/core-checkbox: 1.1.0 => 1.1.1
 - @tds/core-display-heading: 1.0.4 => 1.0.5
 - @tds/core-expand-collapse: 1.2.0 => 1.2.1
 - @tds/core-flex-grid: 2.2.0 => 2.2.1
 - @tds/core-heading: 1.1.3 => 1.1.4
 - @tds/core-input: 1.0.10 => 1.0.11
 - @tds/core-notification: 1.2.0 => 1.2.1
 - @tds/core-ordered-list: 2.0.0 => 2.0.1
 - @tds/core-paragraph: 1.0.2 => 1.0.3
 - @tds/core-price-lockup: 1.0.1 => 1.0.2
 - @tds/core-radio: 1.1.0 => 1.1.1
 - @tds/core-select: 1.0.11 => 1.0.12
 - @tds/core-small: 1.0.2 => 1.0.3
 - @tds/core-spinner: 2.0.2 => 2.0.3
 - @tds/core-step-tracker: 2.0.2 => 2.0.3
 - @tds/core-strong: 1.0.2 => 1.0.3
 - @tds/core-text: 1.0.2 => 1.0.3
 - @tds/core-text-area: 1.0.9 => 1.0.10
 - @tds/core-tooltip: 2.0.2 => 2.0.3
 - @tds/core-unordered-list: 2.0.0 => 2.0.1
 - @tds/shared-choice: 1.0.6 => 1.0.7 (private)
 - @tds/shared-colored-text-provider: 1.0.0-dev => 1.0.0 (private)
 - @tds/shared-form-field: 1.0.7 => 1.0.8 (private)
 - @tds/shared-typography: 1.0.2 => 1.0.3 (private)


🤔 If this is not what you expected, ensure that your commit messages follow the Conventional Commits specification (https://conventionalcommits.org) and try again.

🚀 Please paste the entire output of this task into the body of your Pull Request so that a maintainer can verify before merging/publishing.

Copy link
Contributor

@theetrain theetrain left a comment

Choose a reason for hiding this comment

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

This looks ready for QA 👍

Please wait for Breadcrumbs component to be released before merging. Please squash and merge this PR.

@jraff jraff merged commit 5a64f4e into master Dec 10, 2018
@jraff jraff deleted the fix/priceLockup branch December 10, 2018 16:29
jraff pushed a commit that referenced this pull request Dec 17, 2018
* fix(core-price-lockup): fixing horizontal alignment issue

wrapped a div with inline-block and alignment left to account FlexGrid Row horizontal alignment changes

* fix(core-price-lockup): adjustments to horizontal alignment

removed extra div

* style(core-price-lockup): removing inline-block

removing inline-block and updating screenshots and tests
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.

None yet

4 participants