refactor: remove global carbon components from search component sass files #146

besh · 2020-08-05T17:36:47Z

What do these changes do/fix?

This change removes carbon imports that are within each search component's scss files to remove the chance of duplicate imports that lead to longer build times.

The problem

Consumer app imports base carbon styles for its needs

styles.scss

// Carbon global styles
@import 'carbon-components/scss/globals/scss/_css--font-face.scss';
@import 'carbon-components/scss/globals/scss/_colors.scss';
@import 'carbon-components/scss/globals/scss/_vars.scss';
@import 'carbon-components/scss/globals/scss/_mixins.scss';
@import 'carbon-components/scss/components/ui-shell/_variables.scss';
@import 'carbon-components/scss/components/ui-shell/_content.scss';
@import 'carbon-components/scss/components/ui-shell/_functions.scss';
@import 'carbon-components/scss/components/ui-shell/_header.scss';
@import '~carbon-components/scss/components/search/search';
@import '~carbon-components/scss/components/list-box/list-box';

Consumer app then imports search component styles

styles.scss

// Carbon global styles
@import 'carbon-components/scss/globals/scss/_css--font-face.scss';
@import 'carbon-components/scss/globals/scss/_colors.scss';
@import 'carbon-components/scss/globals/scss/_vars.scss';
@import 'carbon-components/scss/globals/scss/_mixins.scss';
@import 'carbon-components/scss/components/ui-shell/_variables.scss';
@import 'carbon-components/scss/components/ui-shell/_content.scss';
@import 'carbon-components/scss/components/ui-shell/_functions.scss';
@import 'carbon-components/scss/components/ui-shell/_header.scss';
@import 'carbon-components/scss/components/search/search';
@import 'carbon-components/scss/components/list-box/list-box';

// Discovery component styles
@import '@ibm-watson/discovery-styles/scss/index.scss';

However, the discovery component index.scss file also imports carbon global styles

@ibm-watson/discovery-styles/scss/index.scss

@import '~carbon-components/scss/globals/scss/_css--font-face.scss';
@import '~carbon-components/scss/globals/scss/_typography.scss';
@import '~carbon-components/scss/globals/scss/_colors.scss';
@import '~carbon-components/scss/globals/scss/_layout.scss';

Additionally, each discovery component scss file imports the base carbon styles required for the component.

@ibm-watson/discovery-styles/scss/components/search-input/_search-input.scss

@import '~carbon-components/scss/components/search/search';
@import '~carbon-components/scss/components/list-box/list-box';

Because the consumer app is already importing the base carbon styles required for its usage of carbon, anything that is also imported via discovery-components will be imported again, leading to longer build times.

The proposed solution

Follow suite with carbon-react and stay "hands off" on base carbon style imports, but still provide the discovery component styles for consumption by the consumer app.

In this change, I'm removing all global carbon style imports from the discovery components and placing them into a global-carbon-styles file. This allows us to do the following:

Prevents duplicate imports if the consumer app is already importing this files.
Provides an easy way for the consumer app to import these global files if they do want to rely on the discovery-components to provide the global styles needed for these components. This would serve the use case where the consumer app is only interested in using the discovery components and doesn't want to have to figure out which imports are required from carbon while also serving the use case of apps that are using a mixed back of core carbon components plus add-ons.

Basically, instead of the consumer app doing:

@import 'carbon-components/scss/globals/scss/_css--font-face.scss';
@import 'carbon-components/scss/globals/scss/_colors.scss';
@import 'carbon-components/scss/globals/scss/_vars.scss';
@import 'carbon-components/scss/globals/scss/_mixins.scss';
@import 'carbon-components/scss/components/ui-shell/_variables.scss';
@import 'carbon-components/scss/components/ui-shell/_content.scss';
@import 'carbon-components/scss/components/ui-shell/_functions.scss';
@import 'carbon-components/scss/components/ui-shell/_header.scss';
@import 'carbon-components/scss/components/search/search';
@import 'carbon-components/scss/components/list-box/list-box';

@import '@ibm-watson/discovery-styles/scss/index.scss';

it can instead do

@import '@ibm-watson/discovery-styles/scss/carbon-components.scss';
@import '@ibm-watson/discovery-styles/scss/discovery-components.scss';

We can easily import both of these files into story book to continue having all of the carbon base styles available during package development.

How do you test/verify these changes?

Yes. Here is the sample app still running with the correct styles:

Have you documented your changes (if necessary)?

Yes

Are there any breaking changes included in this pull request?

I don't believe so. Because we still export the index.scss file which includes everything the same as before, this should be purely an additive change.

CLAassistant · 2020-08-05T17:36:51Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ besh
❌ Dave Beshero

Dave Beshero seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

besh · 2020-08-05T17:41:10Z

packages/discovery-styles/scss/global-carbon-styles.scss

+
+// Search Input
+@import '~carbon-components/scss/components/search/search';
+@import '~carbon-components/scss/components/list-box/list-box';


This whole file is completely optional for consumer apps. They can choose to import this file along with the base discovery components scss file if they don't plan on mixing in any additional carbon components. I imagine this would be more likely used by users who are quickly building a sample search app. Perhaps as their project grows, they would abandon importing this file in favor of importing from carbon directly. Bottom line is that this allows us to provide a decent developer experience for those who want a quick path to get something going while also saying "hey, you don't need to use this file if you have a more complex setup to manage."

besh · 2020-08-05T17:43:57Z

packages/discovery-styles/scss/global-carbon-styles.scss

@@ -0,0 +1,14 @@
+// You only need to import this file if your app is not already directly importing these carbon files


If we like this approach, I'd write up a better statement here that links to docs including this: https://github.com/carbon-design-system/carbon/blob/master/docs/guides/sass.md#optimizing-your-sass-builds

maniax89 · 2020-08-05T17:57:04Z

packages/discovery-styles/scss/components/search-input/_search-input.scss

@@ -1,7 +1,3 @@
-// Component styles
-@import '~carbon-components/scss/components/search/search';


does @use work here? https://css-tricks.com/introducing-sass-modules/#:~:text=The%20new%20%40use%20is%20similar,considered%20private%2C%20and%20not%20imported.

or can we at least leave a comment indicating which carbon styles this file depends on?

It seems like it would work, but we would have to use dart-sass instead of node-sass which then requires anyone consuming this package to also use dart-sass.

At this point in time, it doesn't appear that @use is on the node-sass roadmap: sass/node-sass#2886 (comment)

@maniax89 would you prefer comments per file or references within this file:

discovery-components/packages/discovery-styles/scss/global-carbon-styles.scss

Line 12 in 62561bc

// Search Input

either or - i don't have a preference. just as long as it is consistent

Seems like many components import the same base styles. For example

https://github.com/watson-developer-cloud/discovery-components/blob/master/packages/discovery-styles/scss/components/ci-document/_ci-document-filter.scss#L1-L3

and

https://github.com/watson-developer-cloud/discovery-components/blob/master/packages/discovery-styles/scss/components/search-facets/_search-facets.scss#L1-L3

Thinking that I'll go the route of putting all of the comments in the global file and include a section of shared base carbon styles across components.

Did that here: https://github.com/watson-developer-cloud/discovery-components/pull/146/files#diff-bb1e94d2444f11bfc036c8c60f806d24

maniax89

did you update the example app as well? https://github.com/watson-developer-cloud/discovery-components/blob/master/examples/discovery-search-app/src/index.scss#L7

besh · 2020-08-06T11:44:29Z

Going to be doing that today before I move this out of draft. Also will be updating docs.

besh · 2020-08-11T16:47:52Z

README.md

+
+The Discovery Components styles package can be consumed in three ways
+
+1. Wholesale SCSS


Not married to the definitions here. Just wanted something to quickly describe what the method is. Maybe single import SCSS and partial SCSS imports instead?

README.md

besh commented Aug 5, 2020

View reviewed changes

maniax89 reviewed Aug 5, 2020

View reviewed changes

besh force-pushed the refactor/scss-imports branch from 8cae616 to 5ebe015 Compare August 5, 2020 21:39

maniax89 reviewed Aug 5, 2020

View reviewed changes

Dave Beshero added 2 commits August 10, 2020 12:19

refactor: initial draft of scss imports

80c9743

chore: the rest of the ccomponents

743423c

besh force-pushed the refactor/scss-imports branch from 5ebe015 to 130fee8 Compare August 10, 2020 20:02

chore: update style paths

3711b47

besh force-pushed the refactor/scss-imports branch from 130fee8 to 3711b47 Compare August 10, 2020 20:03

refactor: rollup to index.scss for css build

bd963e3

besh marked this pull request as ready for review August 11, 2020 16:18

besh requested review from broulaye, ChrisGillin, JohnWColby, jhpedemonte, kabeaty, noah-eigenfeld and Steven-Gassert as code owners August 11, 2020 16:18

besh force-pushed the refactor/scss-imports branch from 35a3bb4 to de50ffc Compare August 11, 2020 16:22

docs: add CSS import info

4d419ee

besh force-pushed the refactor/scss-imports branch from de50ffc to 4d419ee Compare August 11, 2020 16:24

besh commented Aug 11, 2020

View reviewed changes

jhpedemonte approved these changes Aug 11, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

maniax89 approved these changes Aug 11, 2020

View reviewed changes

docs: update Optimize CSS title

b48a722

maniax89 merged commit f70b2b0 into watson-developer-cloud:master Aug 12, 2020

besh deleted the refactor/scss-imports branch August 12, 2020 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: remove global carbon components from search component sass files #146

refactor: remove global carbon components from search component sass files #146

besh commented Aug 5, 2020 •

edited

Loading

CLAassistant commented Aug 5, 2020 •

edited

Loading

besh Aug 5, 2020 •

edited

Loading

besh Aug 5, 2020

maniax89 Aug 5, 2020

besh Aug 5, 2020

besh Aug 5, 2020

maniax89 Aug 5, 2020

besh Aug 5, 2020

besh Aug 5, 2020

maniax89 left a comment

besh commented Aug 6, 2020

besh Aug 11, 2020

		@@ -0,0 +1,14 @@
		// You only need to import this file if your app is not already directly importing these carbon files

		@@ -1,7 +1,3 @@
		// Component styles
		@import '~carbon-components/scss/components/search/search';


		The Discovery Components styles package can be consumed in three ways

		1. Wholesale SCSS

refactor: remove global carbon components from search component sass files #146

refactor: remove global carbon components from search component sass files #146

Conversation

besh commented Aug 5, 2020 • edited Loading

What do these changes do/fix?

The problem

The proposed solution

How do you test/verify these changes?

Have you documented your changes (if necessary)?

Are there any breaking changes included in this pull request?

CLAassistant commented Aug 5, 2020 • edited Loading

besh Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maniax89 left a comment

Choose a reason for hiding this comment

besh commented Aug 6, 2020

Choose a reason for hiding this comment

besh commented Aug 5, 2020 •

edited

Loading

CLAassistant commented Aug 5, 2020 •

edited

Loading

besh Aug 5, 2020 •

edited

Loading