Refactor addon-action to use glamorous #2275

armandabric · 2017-11-09T10:15:31Z

Hey storybookers,

This PR is related to #2263

This commit convert the Action addon to glamourous and extract the button as a shared component.

I don't known how you want to process:

Convert one addons at the time to glamourous and after extract what should be shared
Convert one addons at the time to glamourous and extract what should be shared at the same time
Convert all addons in the same move and extract what should be shared at the same time

This is my first PR in storybook so I'm probably forgetting somethings ;)

Hypnosphi · 2017-11-09T11:37:53Z

I'd prefer this:
Extract one or two components at the time, move them to lib/components and convert to glamourous. Then use elsewhere (in separate PRs maybe)

Hypnosphi · 2017-11-09T11:46:33Z

lib/components/src/form/button.js

@@ -0,0 +1,10 @@
+import glamorous from 'glamorous';
+
+export default glamorous.button({


Let's set type="button" here. By default buttons are type="submit", but it's never what we actually want.

With glamorous, you can do it like that:

glamorous.button({ <styles> }).withProps({ type: 'button' })

Hypnosphi · 2017-11-09T11:50:21Z

Please run yarn jest -u to update test snapshot

armandabric · 2017-11-09T12:40:02Z

I have update the snapshot, but I do not know my classnames hashes have not the same that the one generated on the CI 😕

Hypnosphi · 2017-11-09T12:59:25Z

That's a bit weird. Please try to run yarn bootstrap --reset --core (this will remove all the gitignored directories like node_modules and reinstall everything from scratch), and update snapshots again

armandabric · 2017-11-09T14:26:15Z

I've run:

$ yarn bootstrap --reset --core
$ yarn jest -u

My snapshots are still the same 😕

Hypnosphi · 2017-11-09T14:39:47Z

What does git status output for you?

armandabric · 2017-11-09T14:45:41Z

That all my change have been commited:

$ git status
On branch qa/extract-guidelines
nothing to commit, working tree clean

I'm on slack if you want to give me a hand directly ✋

Hypnosphi · 2017-11-09T20:27:03Z

lib/components/src/form/button.js

+    padding: '5px 10px',
+    borderRadius: '4px',
+    color: 'rgba(0, 0, 0, 0.5)',
+    outline: 'none',


Let's either not remove it in a generic button, or provide something instead, using ':focus': {...styles}

We could add some :hover and :active states as well

Hypnosphi · 2017-11-10T12:43:11Z

@ndelangen are you familiar with how glamor classNames are generated? Why can it be, for example, css-1fqbdip for me and CI, and css-glamorous-article for @Spy-Seth ?

ndelangen · 2017-11-10T20:24:46Z

I've never experienced this.. Maybe NODE_ENV? Maybe we can ask @kentcdodds

Looks like one of the 2 is optimized and the other isn't.
I can't find any documentation on this on https://glamorous.rocks/advanced#optimizing-bundle-size

This is what I found:
https://github.com/threepointone/glamor#speedy-mode

kentcdodds · 2017-11-10T20:26:18Z

Yes, glamor will apply the label (which is the displayName of the component) if NODE_ENV !== 'production' so that's possibly the problem.

codecov · 2017-11-11T00:07:05Z

Codecov Report

Merging #2275 into master will increase coverage by 0.06%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master    #2275      +/-   ##
==========================================
+ Coverage   37.48%   37.54%   +0.06%     
==========================================
  Files         434      428       -6     
  Lines        9389     9176     -213     
  Branches      908      890      -18     
==========================================
- Hits         3519     3445      -74     
+ Misses       5301     5192     -109     
+ Partials      569      539      -30

Impacted Files	Coverage Δ
.../ui/src/modules/ui/components/addon_panel/style.js	`0% <ø> (ø)`	⬆️
lib/components/src/index.js	`0% <0%> (ø)`	⬆️
lib/components/src/form/button.stories.js	`100% <100%> (ø)`
lib/components/src/navigation/menu_link.stories.js	`100% <100%> (ø)`	⬆️
...b/components/src/navigation/routed_link.stories.js	`100% <100%> (ø)`	⬆️
...ddons/actions/src/components/ActionLogger/style.js	`100% <100%> (ø)`	⬆️
lib/components/src/form/button.js	`100% <100%> (ø)`
...ddons/actions/src/components/ActionLogger/index.js	`73.68% <80%> (-0.68%)`	⬇️
app/react/src/server/utils.js	`0% <0%> (-100%)`	⬇️
addons/notes/src/index.js	`0% <0%> (-61.54%)`	⬇️
... and 101 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a0fc1b...f87336c. Read the comment docs.

# Conflicts: # addons/actions/package.json # examples/cra-kitchen-sink/src/stories/__snapshots__/addon-info.stories.storyshot # lib/components/src/index.js

Hypnosphi · 2017-11-11T00:55:57Z

lib/components/src/form/button.js

+    background: 'rgba(255, 255, 255, 0.5)',
+    padding: '5px 10px',
+    borderRadius: '4px',
+    color: 'rgba(0, 0, 0, 0.5)',


It looks disabled to me as it is now:

Hypnosphi · 2017-11-11T01:00:00Z

lib/components/src/form/button.js

+  .button({
+    border: 'solid 1px rgba(0, 0, 0, 0.2)',
+    background: 'rgba(255, 255, 255, 0.5)',
+    padding: '5px 10px',


Let's add some more padding at the bottom, so that the text is vertically centered by lowercase letters. 4px 10px 7px seems to be enough:

ndelangen · 2017-11-11T10:22:05Z

Thanks for the assistance @kentcdodds! 👏

@Spy-Seth can you try finding out why it would appear your node is in production mode when running tests?

armandabric · 2017-11-13T08:22:40Z

It seems that @kentcdodds have made the good guess 👍

$ printenv NODE_ENV
development

If I set it to testing the generated hashes are optimized again 🎉 I will upgrade the PR with the good one. (EDIT: it seems that @Hypnosphi already did it!)

@ndelangen: Now I'm able to take some times in the next days to apply the @Hypnosphi comments.

Thanks everyone for you're help 👍

Hypnosphi · 2017-11-13T11:31:58Z

If I set it to testing

Actually, it should rather be test (this is what jest sets by default when there's no such variable)

ndelangen · 2017-12-23T22:59:20Z

@Spy-Seth anything you need to continue?

armandabric · 2017-12-30T21:50:57Z

Yeah: times 😅 Like all of us

I was stuck on a live reload issue with the monorepo. I could not exactly remember the details sorry. I will have some time in the beginning of 2018. I will try to get back on it but I could not guaranty it. If someone want to take it and finish the move it's fine for me 👍

Hypnosphi · 2018-02-17T12:56:00Z

OK I think I'll pick it

# Conflicts: # addons/actions/package.json # examples/cra-kitchen-sink/src/stories/__snapshots__/storybook-components.stories.storyshot # examples/cra-kitchen-sink/src/stories/storybook-components.stories.js

Hypnosphi · 2018-02-17T15:36:53Z

Ready for review

…ub.com/dependencies-io/actor-js-lerna/pull/37 gets merged

self-review

armandabric changed the title ~~Create a collection of reusable base element~~ [WIP] Create a collection of reusable base element Nov 9, 2017

Create a collection of reusable base element

264d9bd

Hypnosphi requested changes Nov 9, 2017

View reviewed changes

Fix review comments

436c70e

danielduan added the in progress label Nov 9, 2017

Hypnosphi added components addon: actions feature request labels Nov 9, 2017

Update test snapshots

5c3e94c

Hypnosphi reviewed Nov 9, 2017

View reviewed changes

Hypnosphi changed the base branch from master to release/3.3 November 9, 2017 21:38

Hypnosphi added 2 commits November 11, 2017 03:42

Merge branch 'release/3.3' into Spy-Seth-qa/extract-guidelines

a727da5

# Conflicts: # addons/actions/package.json # examples/cra-kitchen-sink/src/stories/__snapshots__/addon-info.stories.storyshot # lib/components/src/index.js

Update storyshot

7fea872

Hypnosphi requested changes Nov 11, 2017

View reviewed changes

ndelangen assigned armandabric and Hypnosphi Nov 11, 2017

Update storyshot

d8dacf6

ndelangen changed the base branch from release/3.3 to master December 23, 2017 22:57

Hypnosphi unassigned armandabric Feb 17, 2018

Hypnosphi added the maintenance User-facing maintenance tasks label Feb 17, 2018

Hypnosphi added 3 commits February 17, 2018 16:56

Merge remote-tracking branch 'origin/master' into qa/extract-guidelines

ba11829

# Conflicts: # addons/actions/package.json # examples/cra-kitchen-sink/src/stories/__snapshots__/storybook-components.stories.storyshot # examples/cra-kitchen-sink/src/stories/storybook-components.stories.js

Re-add button story, update storyshots

b76d07a

Improve button styles

f62a4d6

Hypnosphi added cleanup Minor cleanup style change that won't show up in release changelog and removed in progress labels Feb 17, 2018

Hypnosphi changed the title ~~[WIP] Create a collection of reusable base element~~ Refactor addon-action to use glamorous Feb 17, 2018

Hypnosphi requested a review from a team February 17, 2018 15:36

Hypnosphi previously approved these changes Feb 17, 2018

View reviewed changes

Hypnosphi requested a review from a team February 17, 2018 15:37

Hypnosphi and others added 2 commits February 17, 2018 18:45

Dependencies.io: temporary disable batch_bootstrap until https://gith…

b1aa7e1

…ub.com/dependencies-io/actor-js-lerna/pull/37 gets merged

Merge branch 'master' into qa/extract-guidelines

6da77c9

Hypnosphi added the ready label Feb 20, 2018

danielduan approved these changes Feb 20, 2018

View reviewed changes

Merge branch 'master' into qa/extract-guidelines

f87336c

Hypnosphi merged commit 3edc7d1 into storybookjs:master Feb 20, 2018

armandabric deleted the qa/extract-guidelines branch February 23, 2018 11:53

Hypnosphi mentioned this pull request Feb 24, 2018

Addons: avoid mixing manager and preview code together #3068

Merged

dependencies bot mentioned this pull request May 14, 2018

Update 80 dependencies from npm #3584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor addon-action to use glamorous #2275

Refactor addon-action to use glamorous #2275

armandabric commented Nov 9, 2017 •

edited

Loading

Hypnosphi commented Nov 9, 2017 •

edited

Loading

Hypnosphi Nov 9, 2017

Hypnosphi commented Nov 9, 2017

armandabric commented Nov 9, 2017 •

edited

Loading

Hypnosphi commented Nov 9, 2017

armandabric commented Nov 9, 2017 •

edited

Loading

Hypnosphi commented Nov 9, 2017

armandabric commented Nov 9, 2017

Hypnosphi Nov 9, 2017 •

edited

Loading

Hypnosphi Nov 11, 2017

Hypnosphi commented Nov 10, 2017

ndelangen commented Nov 10, 2017

kentcdodds commented Nov 10, 2017

codecov bot commented Nov 11, 2017 •

edited

Loading

Hypnosphi Nov 11, 2017

Hypnosphi Nov 11, 2017

ndelangen commented Nov 11, 2017

armandabric commented Nov 13, 2017 •

edited

Loading

Hypnosphi commented Nov 13, 2017

ndelangen commented Dec 23, 2017

armandabric commented Dec 30, 2017

Hypnosphi commented Feb 17, 2018

Hypnosphi commented Feb 17, 2018

		@@ -0,0 +1,10 @@
		import glamorous from 'glamorous';

		export default glamorous.button({

Refactor addon-action to use glamorous #2275

Refactor addon-action to use glamorous #2275

Conversation

armandabric commented Nov 9, 2017 • edited Loading

Hypnosphi commented Nov 9, 2017 • edited Loading

Hypnosphi Nov 9, 2017

Choose a reason for hiding this comment

Hypnosphi commented Nov 9, 2017

armandabric commented Nov 9, 2017 • edited Loading

Hypnosphi commented Nov 9, 2017

armandabric commented Nov 9, 2017 • edited Loading

Hypnosphi commented Nov 9, 2017

armandabric commented Nov 9, 2017

Hypnosphi Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Hypnosphi Nov 11, 2017

Choose a reason for hiding this comment

Hypnosphi commented Nov 10, 2017

ndelangen commented Nov 10, 2017

kentcdodds commented Nov 10, 2017

codecov bot commented Nov 11, 2017 • edited Loading

Codecov Report

Hypnosphi Nov 11, 2017

Choose a reason for hiding this comment

Hypnosphi Nov 11, 2017

Choose a reason for hiding this comment

ndelangen commented Nov 11, 2017

armandabric commented Nov 13, 2017 • edited Loading

Hypnosphi commented Nov 13, 2017

ndelangen commented Dec 23, 2017

armandabric commented Dec 30, 2017

Hypnosphi commented Feb 17, 2018

Hypnosphi commented Feb 17, 2018

armandabric commented Nov 9, 2017 •

edited

Loading

Hypnosphi commented Nov 9, 2017 •

edited

Loading

armandabric commented Nov 9, 2017 •

edited

Loading

armandabric commented Nov 9, 2017 •

edited

Loading

Hypnosphi Nov 9, 2017 •

edited

Loading

codecov bot commented Nov 11, 2017 •

edited

Loading

armandabric commented Nov 13, 2017 •

edited

Loading