Core: Add new layout style none and fix layout styles
#12727
Conversation
guilhermewaess
requested review from
alterx,
igor-dv and
tmeasday
as code owners
guilhermewaess
force-pushed
the
refactor/storyrenderer-styles
branch
from
922c952 to
bc286be
Compare
guilhermewaess
requested review from
ndelangen and
theinterned
as code owners
guilhermewaess
force-pushed
the
refactor/storyrenderer-styles
branch
from
bc286be to
66138e3
Compare
shilman
changed the title
none
shilman
changed the title
nonenone and fix layout styles
There was a problem hiding this comment.
Great stuff! I'd like @ghengeveld to review since he introduced the layout parameter in 6.0
|
Great work @guilhermewaess! I've gone through the changes and tested them, seems great! |
| Object.assign(document.body.style, styles || {}); | ||
| this.previousStyles = styles; | ||
| checkIfLayoutExists(layout: string) { | ||
| if (!Object.keys(layoutClassMap).includes(layout)) { |
There was a problem hiding this comment.
Any reason why not to use if (!layoutClassMap[layout]) ? Maybe TypeScript doesn't allow it?
There was a problem hiding this comment.
Good thing you added this warning by the way 👍
There was a problem hiding this comment.
should make no difference to TS
There was a problem hiding this comment.
Good catch @ghengeveld .. @guilhermewaess @yannbf ?
There was a problem hiding this comment.
Yes, I agree it's better to do !layoutClassMap[layout], will change it tonight :)
Remove logic from adding styles through styles attr and start to use css instead.
guilhermewaess
force-pushed
the
refactor/storyrenderer-styles
branch
from
dd296a2 to
21ec3c9
Compare
Issue: #12434 #12041
This PR: Removes the logic from adding styles on body element through styles attr.
Adds a new option for layout: none.
What I did
Added a new option for layouts called none, which won't apply any new style on preview body.
Moved the layouts styles from js to CSS.
Fixed CSS initials override.
How to test
Change the layout property on story parameters to one of the available layouts:
example:
None.parameters = { layout: 'none' };Is this testable with Jest or Chromatic screenshots?
I don't think it will break any story.
Does this need a new example in the kitchen sink apps?
It's added.
Does this need an update to the documentation?
Yes, but I didn't find any docs related to available parameters.
If your answer is yes to any of these, please make sure to include it in your PR.
Discussion
When the received layout is invalid, what should be the proper behaviour?
For now, it is showing a warning on the console with the invalid option and telling the available options. However, this solution also adds an
undefinedas a class on the body.Thanks to @yannbf for the guidance :D