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: fix that some styles doesn’t work #705

Merged
merged 1 commit into from
Mar 24, 2020
Merged

fix: fix that some styles doesn’t work #705

merged 1 commit into from
Mar 24, 2020

Conversation

im36-123
Copy link
Contributor

@im36-123 im36-123 commented Mar 24, 2020

Currently, some styles of Cell component has problem, because of missing ;.
This problem became apparent with the update of Styled-components. There seems to be a difference in output between v4 and v5.

Input
Note: some properties have no ;

const Th = styled.th<{ themes: Theme}>`
  ${({ themes}) => {
    const { size, palette, interaction } = themes

    return css`
      font-size: ${size.pxToRem(size.font.SHORT)}
      font-weight: bold;
      padding: ${size.pxToRem(size.space.XS)};
      color: ${palette.TEXT_GREY};
      transition: ${isTouchDevice ? 'none' : `background-color ${interaction.hover.animation}`}
      text-align: left;
      line-height: 1.5;
      vertical-align: middle;
    `
  }}
`

Output (v4)

.foo {
    font-size: 0.6875rem;
    font-weight: bold;
    padding: 1rem;
    color: #767676;
    -webkit-transition: background-color .3s ease-out;
    transition: background-color .3s ease-out;
    text-align: left;
    line-height: 1.5;
    vertical-align: middle;
}

Output (v5)
font-size and transition have wrong value.

.foo {
    font-size: 0.6875rem font-weight:bold;
    padding: 1rem;
    color: #767676;
    -webkit-transition: background-color .3s ease-out text-align:left;
    transition: background-color .3s ease-out text-align:left;
    line-height: 1.5;
    vertical-align: middle;
}

Lines without ; seem to be convert to value until the next ; (I think this is the correct behavior).

So, I added ; to enable properties correctly.

@im36-123 im36-123 requested a review from nabeliwo March 24, 2020 02:55
@im36-123 im36-123 requested a review from a team as a code owner March 24, 2020 02:55
@reg-suit
Copy link

reg-suit bot commented Mar 24, 2020

reg-suit detected visual differences.

Check this report, and review them.

🔴

🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵

What do the circles mean? The number of circles represent the number of changed images.
🔴 : Changed items, ⚪ : New items, ⚫ : Deleted items, and 🔵 Passed items

How can I change the check status? If reviewers approve this PR, the reg context status will be green automatically.

@netlify
Copy link

netlify bot commented Mar 24, 2020

Deploy preview for smarthr-ui ready!

Built with commit 9f2680d

https://deploy-preview-705--smarthr-ui.netlify.com

@im36-123 im36-123 changed the title [WIP] fix: fix that some styles doesn’t work fix: fix that some styles doesn’t work Mar 24, 2020
Copy link
Member

@nabeliwo nabeliwo left a comment

Choose a reason for hiding this comment

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

Thank you for your great contribution!! LGTM 🎉

@im36-123
Copy link
Contributor Author

Thanks for your review!!

@im36-123 im36-123 merged commit f6f75a6 into master Mar 24, 2020
@im36-123 im36-123 deleted the fix-style branch March 24, 2020 08:39
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