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.

Sign up for GitHub

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

Jump to bottom

fix(form/login/wizard): fixed undefined issues with IE11 script #1871

Merged
merged 5 commits into from May 30, 2019
Merged

fix(form/login/wizard): fixed undefined issues with IE11 script #1871

merged 5 commits into from May 30, 2019

Conversation

dlabaj
Copy link
Contributor

@dlabaj dlabaj commented May 30, 2019 •
edited

Fixed order of variables which was causing the IE conversion script to generate undefined. Also removed a login variable that was not needed. This was also causing the IE scripts to generate undefined.

This resolves issue: #1869

@patternfly-build
Copy link

patternfly-build commented May 30, 2019 •
edited

Deploy preview for pf-next ready!

Built with commit 4cb5b0c

https://deploy-preview-1871--pf-next.netlify.com

seanforyou23
seanforyou23 approved these changes May 30, 2019
View reviewed changes
Copy link
Collaborator

@seanforyou23 seanforyou23 left a comment

Choose a reason for hiding this comment

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

👍

mcoker
mcoker reviewed May 30, 2019
View reviewed changes
src/patternfly/components/Wizard/wizard.scss
@@ -59,7 +64,6 @@

// Toggle list
--pf-c-wizard__toggle-list--MarginRight: var(--pf-global--spacer--sm);
--pf-c-wizard__toggle-list--MarginBottom: calc(var(--pf-c-wizard__toggle-list-item--MarginBottom) * -1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to remove this? Seems like you want to just move it below where --pf-c-wizard__toggle-list-item--MarginBottom is defined.

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 did mean to remove it because this one also came out to be zero. I'll double check to make sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's -4px for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah my mistake thanks will add it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

mcoker
mcoker reviewed May 30, 2019
View reviewed changes
src/patternfly/components/Wizard/wizard.scss Outdated
@@ -270,7 +271,6 @@
flex-wrap: wrap;
align-items: baseline;
margin-right: var(--pf-c-wizard__toggle-list--MarginRight);
margin-bottom: var(--pf-c-wizard__toggle-list--MarginBottom);
Copy link
Contributor

Choose a reason for hiding this comment

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

Then add this back if you add --pf-c-wizard__toggle-list--MarginBottom back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

mcoker
mcoker reviewed May 30, 2019
View reviewed changes
src/patternfly/components/Wizard/wizard.scss
@@ -115,9 +119,6 @@
--pf-c-wizard__nav-link--m-disabled--Color: var(--pf-global--Color--dark-200);

// Nav link number
--pf-c-wizard__nav-link--before--Width: 1.5rem;
--pf-c-wizard__nav-link--before--Height: 1.5rem;
--pf-c-wizard__nav-link--before--Top: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of moving just these 3 lines, can you move this whole block up so that the // Nav link number CSS is all in one spot?

srambach
srambach reviewed May 30, 2019
View reviewed changes
src/patternfly/components/Login/login.scss
@@ -76,7 +76,6 @@
--pf-c-login__main-footer-links--PaddingRight: var(--pf-global--spacer--3xl);
--pf-c-login__main-footer-links--PaddingBottom: var(--pf-global--spacer--xl);
--pf-c-login__main-footer-links--PaddingLeft: var(--pf-global--spacer--3xl);
--pf-c-login__main-footer-links--MarginBottom: calc(var(--pf-c-login__main-footer-links--MarginBottom) * -1);
Copy link
Member

Choose a reason for hiding this comment

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

Same question as @mcoker had on removing --pf-c-wizard__toggle-list--MarginBottom from wizard.scss - did you mean to remove it completely or just move this line below --pf-c-login__main-footer-links--MarginBottom?

Copy link
Member

Choose a reason for hiding this comment

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

Wait, nevermind - I see this is actually self-referential?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. If you look now the margin there just computes to 0 since --pf-c-login__main-footer-links--MarginBottom isn't defined anywhere else.

Copy link
Member

Choose a reason for hiding this comment

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

As long as this wasn't a mistake where it really should have been assigned to another variable. I'm not sure why it would have been put in there that way. But, I don't know what the original specs were.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I looked at the commit where this was introduced (#1281) and looks like a mistake. The layout matches the design in sketch, too.

@dlabaj
Copy link
Contributor Author

dlabaj commented May 30, 2019

@srambach and @mcoker Thanks for the feedback. I think it's good now. Let me know if you see anything else.

mcoker
mcoker reviewed May 30, 2019
View reviewed changes
src/patternfly/components/Wizard/wizard.scss
@@ -106,18 +117,7 @@
// Nav item
--pf-c-wizard__nav-item--MarginTop: var(--pf-global--spacer--md);

// Nav link
Copy link
Contributor

@mcoker mcoker May 30, 2019 •
edited

Choose a reason for hiding this comment

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

Oh you moved this block instead of the // Nav link number block

It should be

  // Nav link
  --pf-c-wizard__nav-link--Color: var(--pf-global--Color--100);
  --pf-c-wizard__nav-link--TextDecoration: var(--pf-global--link--TextDecoration);
  --pf-c-wizard__nav-link--hover--Color: var(--pf-global--link--Color);
  --pf-c-wizard__nav-link--focus--Color: var(--pf-global--link--Color);
  --pf-c-wizard__nav-link--m-current--Color: var(--pf-global--link--Color);
  --pf-c-wizard__nav-link--m-disabled--Color: var(--pf-global--Color--dark-200);

  // Nav link number
  --pf-c-wizard__nav-link--before--Width: 1.5rem;
  --pf-c-wizard__nav-link--before--Height: 1.5rem;
  --pf-c-wizard__nav-link--before--Top: 0;
  --pf-c-wizard__nav-link--before--BackgroundColor: var(--pf-global--BackgroundColor--300);
  --pf-c-wizard__nav-link--before--BorderRadius: var(--pf-global--BorderRadius--lg);
  --pf-c-wizard__nav-link--before--Color: var(--pf-global--Color--100);
  --pf-c-wizard__nav-link--before--FontSize: var(--pf-global--FontSize--sm);
  --pf-c-wizard__nav-link--before--Transform: translateX(calc(-100% - var(--pf-global--spacer--sm)));
  --pf-c-wizard__nav-link--m-current--before--BackgroundColor: var(--pf-global--active-color--100);
  --pf-c-wizard__nav-link--m-current--before--Color: var(--pf-global--Color--light-100);
  --pf-c-wizard__nav-link--m-disabled--before--BackgroundColor: transparent;
  --pf-c-wizard__nav-link--m-disabled--before--Color: var(--pf-global--Color--dark-200);

mcoker
mcoker approved these changes May 30, 2019
View reviewed changes
Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

👍

@mcoker mcoker merged commit e6dd8a0 into patternfly:master May 30, 2019
@patternfly-build
Copy link

🎉 This PR is included in version 2.8.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@redallen
Copy link
Contributor

redallen commented Jun 3, 2019

You all are crazy fast with this, thanks for helping my #1876 :)

@mcoker mcoker mentioned this pull request Jun 6, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srambach srambach srambach left review comments

@seanforyou23 seanforyou23 seanforyou23 approved these changes

@mcoker mcoker mcoker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dlabaj @patternfly-build @redallen @seanforyou23 @srambach @mcoker