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

Remove styled-components (closes #56) #113

Merged
merged 8 commits into from
Mar 16, 2021

Conversation

Gustl22
Copy link
Contributor

@Gustl22 Gustl22 commented Mar 7, 2021

Fix issues

Description

  • Remove styled-components
    • Also adapted docs accordingly
    • Additionally kept classnames, just in case someone uses these in an extra stylesheet.
  • Update storybook
  • Fix and document tests
    • Some tests for ColorButton cannot performed completely, most likely due to: Cascading of inline styles jsdom/jsdom#1696 and Internal/embedded <style> tags don't cascade correctly jestjs/jest#8464
    • See also explanation in code, I really spent days to find a solution, also tried Enzyme, like described in MUI Testing. But this only refers to the properties react components themselves, and don't consider the styles of classes in the rendered result I handed over one step before. And I think it doesn't make any sense to test properties of a component, although the actual css style, which is displayed is the critical part to check. This may comes down to visual testing (?), if it's worth the effort.
    • I also went through the "solution" / code of styled-components and they actually save their style-rules for each class and look them up if needed, e.g. when testing. As far as I understood they actually not loading the style rules from the DOM.

I actually only went through the storybook and checked if anything jumped into my eye. Not sure if I covered all cases a 100%.

Check List

  • respect code conventions and usages
  • tests and 100% coverage, (yes, but depends, more a wontfix issue)
  • well documented
  • self review

Review targets:

  • tests / storybook / styles

@mikbry
Copy link
Owner

mikbry commented Mar 8, 2021

Thanks @Gustl22 for your hard work! I review your PR ASAP

Copy link
Owner

@mikbry mikbry left a comment

Choose a reason for hiding this comment

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

From my tests, I see one change: button stay focused and highlighted which was not the case previously. See the blue highlight
Screenshot 2021-03-09 at 13 37 16
Instead of
Screenshot 2021-03-09 at 13 39 34
Compare using:
https://mikbry.github.io/material-ui-color/?path=/story/components-colorpalette--basic

Copy link
Owner

@mikbry mikbry left a comment

Choose a reason for hiding this comment

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

Some linting/prettier errors i got using VSCode

src/components/ColorPicker.jsx Outdated Show resolved Hide resolved
src/components/ColorBox/index.jsx Outdated Show resolved Hide resolved
@Gustl22
Copy link
Contributor Author

Gustl22 commented Mar 14, 2021

Sorry I'll make the changes the upcoming week. Thx for reviewing :)

@Gustl22
Copy link
Contributor Author

Gustl22 commented Mar 16, 2021

From my tests, I see one change: button stay focused and highlighted which was not the case previously. See the blue highlight
https://mikbry.github.io/material-ui-color/?path=/story/components-colorpalette--basic

Sorry can you explain more detailed? On your mentioned deployed storybook colorpalette, I also get a highlight after clicking on it (tested on Chrome and Firefox).
Feel free to alter my code, if don't want to wait that long :)

@mikbry
Copy link
Owner

mikbry commented Mar 16, 2021

I just tested again and all is fine ;-)

@mikbry
Copy link
Owner

mikbry commented Mar 16, 2021

There is one conflict in yarn.lock, do you fix it or i do ? after that i will merge

@Gustl22
Copy link
Contributor Author

Gustl22 commented Mar 16, 2021

Sure, I can do it quickly :)

@mikbry
Copy link
Owner

mikbry commented Mar 16, 2021

Ok, do it ;-)

@Gustl22
Copy link
Contributor Author

Gustl22 commented Mar 16, 2021

Sorry, I found one more issue with the transparent palette. I'll let you know, when ready :)

@Gustl22 Gustl22 changed the title Remove styled-components (closes #56) WIP: Remove styled-components (closes #56) Mar 16, 2021
@Gustl22 Gustl22 changed the title WIP: Remove styled-components (closes #56) Remove styled-components (closes #56) Mar 16, 2021
@Gustl22
Copy link
Contributor Author

Gustl22 commented Mar 16, 2021

Ready now 😅
Apparently styled-components behave differently from makestyles, when using undefined value (remove attribute vs. leave old value), so I set a default value for background-color and background-image.

I also completely refreshed the yarn.lock, if thats ok, in order to not mess up the previous conflicts.

@mikbry mikbry merged commit 8e7d281 into mikbry:master Mar 16, 2021
@mikbry
Copy link
Owner

mikbry commented Mar 16, 2021

Well done, thanks @Gustl22 !

@mikbry
Copy link
Owner

mikbry commented Mar 16, 2021

Ouch I found a bug in this PR, displaying erroneous color:
in ColorPicker

  • change hex textfield to 'xxx'
  • the red error is mispositionned
    Screenshot 2021-03-16 at 16 46 57

@Gustl22 Gustl22 deleted the styled-component-test branch March 16, 2021 17:14
Gustl22 added a commit to Gustl22/material-ui-color that referenced this pull request Mar 16, 2021
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

2 participants