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

Self contained styles (+pptr) #387

Merged
merged 13 commits into from
Jan 7, 2019
Merged

Self contained styles (+pptr) #387

merged 13 commits into from
Jan 7, 2019

Conversation

montezume
Copy link
Contributor

@montezume montezume commented Jan 4, 2019 •
edited
Loading

Summary

  • Adds pptr-testing-library), which allows us to do dom-testing-library (think RTL) style queries with puppeteer.
  • Encapsulates all styles (Closes Self-contained styles #185)
  • Bug fix: Text.Detail no longer "jumps" down when you set it to inline. (this lead to some tiny vrt diffs)

emmenko
emmenko approved these changes Jan 4, 2019
View reviewed changes
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

💯

materials/internals/reset.mod.css Outdated
@@ -9,24 +9,9 @@
html,
body {
color: var(--color-black);
font-family: 'Open Sans', sans-serif;
font-size: 13px;
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated but should we consider using rem for sizes instead of hard-coded pixels?

package.json
@@ -186,6 +186,7 @@
"postcss-preset-env": "6.5.0",
"postcss-reporter": "6.0.0",
"postcss-safe-parser": "4.0.1",
"pptr-testing-library": "0.3.1",
Copy link
Member

Choose a reason for hiding this comment

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

Nice 🎉

montezume
montezume commented Jan 4, 2019
View reviewed changes
src/components/buttons/secondary-button/secondary-button.visualspec.js
await expect(page).toMatch('A label text');
const doc = await getDocument(page);
const button = await getByLabelText(doc, 'A label text');
expect(button).toBeTruthy();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

feels a bit more like our style of testing 😆

tdeekens
tdeekens approved these changes Jan 4, 2019
View reviewed changes
Copy link
Contributor

@tdeekens tdeekens left a comment

Choose a reason for hiding this comment

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

Nice.

@montezume montezume self-assigned this Jan 4, 2019
dferber90
dferber90 approved these changes Jan 4, 2019
View reviewed changes
Copy link
Contributor

@dferber90 dferber90 left a comment

Choose a reason for hiding this comment

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

pptr 🔥

playground/src/playground.css Outdated
body {
margin: 0;
padding: 0;
height: 100vh;
Copy link
Contributor

@dferber90 dferber90 Jan 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Is this height: 100vh needed? As far as I remember it is something the MC needs which somehow ended up in ui-kit, but our playground should not need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure, I just added it so it looked the same as before 🤷‍♂️

@montezume
Copy link
Contributor Author

Going to do more encapsulate in this PR since I'm on a roll.

@montezume montezume added the 🚧 Status: WIP Work in progress label Jan 4, 2019
tdeekens
tdeekens reviewed Jan 4, 2019
View reviewed changes
src/index.js
@@ -1,6 +1,5 @@
/* This is the entry point of the public package interface */

import '../materials/internals/reset.mod.css';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate how we can be sure that no app-code implicitly depends on these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because both of those files are duplicated in the app shell and are imported there

https://github.com/commercetools/merchant-center-application-kit/blob/master/packages/application-shell/src/components/application-shell/reset.mod.css

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😄

@montezume montezume changed the title feat: add pptr, encapsulate more styles Self contained styles (+pptr) Jan 4, 2019
@montezume montezume removed the 🚧 Status: WIP Work in progress label Jan 4, 2019
@montezume
Copy link
Contributor Author

Okay this is ready for review again 🗡

@montezume montezume mentioned this pull request Jan 5, 2019
Merged
@montezume montezume merged commit fa2a1cd into master Jan 7, 2019
@montezume montezume deleted the ml-encap-styles branch January 7, 2019 09:25
This was referenced Jan 7, 2019
Merged
Closed
@emmenko emmenko mentioned this pull request Jan 26, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tdeekens tdeekens tdeekens approved these changes

@emmenko emmenko emmenko approved these changes

@dferber90 dferber90 dferber90 approved these changes

Assignees

@montezume montezume

Labels
🔮 Type: Chore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Self-contained styles
4 participants
@montezume @emmenko @dferber90 @tdeekens