feat(sketch): create shared styles for icon stroke #5744

emyarod · 2020-03-30T17:29:51Z

This PR renames the fill shared style layers and lays the groundwork for specifying shared style type (currently "fill" or "border") to prepare for #4130/#5567 (branched off of #5569)

Changelog

Changed

change shared sync command and generate commands for colors, themes, and icons to use new naming scheme
fix symbol sync test

Testing / Reviewing

Ensure the sketch plugin works as expected and the new shared style names are correct

Plugin can be downloaded and tested here:

carbon-elements.sketchplugin.zip

joshblack

Do we want all colors to be under fill and stroke and no longer want the color / swatch / grade shared styles? Seems like we might need them for backwards-compat in the library? cc @laurenmrice

netlify · 2020-03-31T08:28:06Z

Deploy preview for carbon-elements ready!

Built with commit 1971cae

https://deploy-preview-5744--carbon-elements.netlify.com

netlify · 2020-03-31T08:31:45Z

Deploy preview for carbon-components-react ready!

Built with commit 1971cae

https://deploy-preview-5744--carbon-components-react.netlify.com

laurenmrice · 2020-03-31T15:24:06Z

I want to keep our existing fill shared color styles separate from the border shared styles. They should not be combined into one style, which is how the plugin is in this pr:

Fill styles should be: color / fill / gray / 100
Border styles should be: color / border / gray / 100

This is how I want them to be bucketed.
Fill:

Border:

Ex of sketch library bucketing with this naming convention (notice how fill and border are separate): (to clarify: the bucket should say border not stroke, this was just an example to test this)

joshblack · 2020-03-31T15:31:56Z

@laurenmrice ah okay, sorry I misunderstood!

joshblack · 2020-03-31T15:33:39Z

Seems great to me after the changes @laurenmrice suggested!

emyarod · 2020-03-31T16:22:44Z

just to clarify, this PR does not contain any of the border work yet, it just lays the groundwork for it in a follow up PR, since the style renaming can cause breakage. this is mostly a refactor of the existing plugin

laurenmrice · 2020-03-31T16:28:21Z

I was confused because if you run the plugin that is attached, the fill color styles have been changed to also include a border on them. Before they were fill only and need to stay that way.

example of what I am seeing:

emyarod · 2020-03-31T16:45:05Z

isn't this already happening in the plugin? from what I can tell this behavior is not exclusive to this PR

steps to replicate:

sync colors
generate color page
sync icons
observe border being applied to color swatches

laurenmrice · 2020-03-31T16:49:30Z

From the last time we released the plugin (9 months ago?) It was exporting correctly and had fills only (no borders applied). You can test this by downloading the current IDL Library that has our generated color styles. sketch://add-library/cloud/nwqmk

Example:

If a fill+border is happening in the plugin for the fill styles that we originally have had, then it's a bug.

laurenmrice · 2020-03-31T17:01:56Z

Andrew and I had a webex convo about this. Seems like there was a regression in the plugin, which is a separate issue from what is happening in the PR. Will need to solve the fill+border color style regression separately.

emyarod · 2020-03-31T20:40:02Z

this appears to be a regression but I haven't found the change which caused this. I have a fix if we want to address this separately but it would be helpful to have more context around how this regression came up

joshblack · 2020-03-31T20:46:38Z

The border is coming from Sketch applying it by default if folks haven't changed their default layer styles for rectangles 👍

joshblack

Seems like the generate command might not be working?

joshblack · 2020-03-31T20:48:30Z

packages/sketch/src/tools/sharedStyles.js

-      },
-    ],
-  });
+export function syncColorStyle(document, name, value, type) {


Instead of adding a type to the end of this and updating all the call signatures that exist already, would it make sense to add a syncFillStyle and syncBorderStyle helper and then we can call them separately?

so would those two additional helpers be called by the sharedStyles helpers?

joshblack · 2020-03-31T20:49:13Z

Let me know if I can help out at all with this!

emyarod · 2020-04-01T15:21:38Z

the generate command was still working, it's the layout that changed because of the layer name difference. it should now be the same as the previous behavior

joshblack

Looks great after removing console 👍

joshblack · 2020-04-02T16:23:03Z

packages/sketch/src/commands/colors/generate.js

@@ -19,12 +19,13 @@ export function generate() {
  command('commands/colors/generate', () => {
    const document = Document.getSelectedDocument();
    const page = selectPage(findOrCreatePage(document, 'color'));
-    const sharedStyles = syncColorStyles(document);
+    const sharedStyles = syncColorStyles(document, 'fill');
+    console.log(sharedStyles);


Suggested change

console.log(sharedStyles);

joshblack · 2020-04-03T14:25:23Z

bump @dakahn let us know if you want to go through this or anything!

joshblack · 2020-04-06T23:23:07Z

bump @dakahn let us know if you want to go through this or anything!

joshblack · 2020-04-08T15:13:05Z

This good to go @emyarod? 👀

emyarod · 2020-04-08T15:25:22Z

yes updated the branch to merge

emyarod added the status: ready for review 👀 label Mar 30, 2020

emyarod requested review from joshblack and laurenmrice March 30, 2020 17:29

emyarod requested a review from a team as a code owner March 30, 2020 17:29

ghost requested a review from dakahn March 30, 2020 17:29

emyarod force-pushed the 5692-shared-styles-icon-stroke branch from ef8c90f to f17c190 Compare March 30, 2020 17:33

joshblack reviewed Mar 30, 2020

View reviewed changes

joshblack approved these changes Mar 31, 2020

View reviewed changes

emyarod force-pushed the 5692-shared-styles-icon-stroke branch from 09cee50 to ca3589b Compare March 31, 2020 16:23

laurenmrice approved these changes Mar 31, 2020

View reviewed changes

joshblack suggested changes Mar 31, 2020

View reviewed changes

joshblack reviewed Mar 31, 2020

View reviewed changes

emyarod force-pushed the 5692-shared-styles-icon-stroke branch 2 times, most recently from d42f1bb to e6d987b Compare April 1, 2020 15:20

emyarod force-pushed the 5692-shared-styles-icon-stroke branch from e6d987b to 38a1291 Compare April 1, 2020 15:22

emyarod requested a review from joshblack April 2, 2020 16:11

joshblack approved these changes Apr 2, 2020

View reviewed changes

emyarod force-pushed the 5692-shared-styles-icon-stroke branch from b5b793b to c0df0cb Compare April 2, 2020 19:47

dakahn approved these changes Apr 7, 2020

View reviewed changes

emyarod added 5 commits April 8, 2020 09:52

feat(sketch): create shared styles for icon stroke

a1954b9

fix(generate): destructure swatch name correctly

90d71b2

test(sketch): update tests with shared style type

0e35e94

feat(sharedStyles): add support for border style type

525fae4

chore: remove console.log

1971cae

emyarod force-pushed the 5692-shared-styles-icon-stroke branch from f055955 to 1971cae Compare April 8, 2020 14:53

joshblack removed the status: ready for review 👀 label Apr 8, 2020

emyarod merged commit 3679b73 into carbon-design-system:master Apr 8, 2020

emyarod deleted the 5692-shared-styles-icon-stroke branch April 8, 2020 15:25

emyarod mentioned this pull request Apr 8, 2020

feat(sketch): add pictogram support to plugin #5827

Closed

emyarod mentioned this pull request Feb 8, 2021

fix(sketch): resolve sketch plugin color layer style duplication #7762

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sketch): create shared styles for icon stroke #5744

feat(sketch): create shared styles for icon stroke #5744

emyarod commented Mar 30, 2020

joshblack left a comment

netlify bot commented Mar 31, 2020 •

edited

Loading

netlify bot commented Mar 31, 2020 •

edited

Loading

laurenmrice commented Mar 31, 2020 •

edited

Loading

joshblack commented Mar 31, 2020

joshblack commented Mar 31, 2020

emyarod commented Mar 31, 2020

laurenmrice commented Mar 31, 2020

emyarod commented Mar 31, 2020 •

edited

Loading

laurenmrice commented Mar 31, 2020

laurenmrice commented Mar 31, 2020

emyarod commented Mar 31, 2020

joshblack commented Mar 31, 2020

joshblack left a comment

joshblack Mar 31, 2020

emyarod Apr 1, 2020

joshblack commented Mar 31, 2020

emyarod commented Apr 1, 2020

joshblack left a comment

joshblack Apr 2, 2020

joshblack commented Apr 3, 2020

joshblack commented Apr 6, 2020

joshblack commented Apr 8, 2020

emyarod commented Apr 8, 2020

feat(sketch): create shared styles for icon stroke #5744

feat(sketch): create shared styles for icon stroke #5744

Conversation

emyarod commented Mar 30, 2020

Changelog

Testing / Reviewing

joshblack left a comment

Choose a reason for hiding this comment

netlify bot commented Mar 31, 2020 • edited Loading

netlify bot commented Mar 31, 2020 • edited Loading

laurenmrice commented Mar 31, 2020 • edited Loading

joshblack commented Mar 31, 2020

joshblack commented Mar 31, 2020

emyarod commented Mar 31, 2020

laurenmrice commented Mar 31, 2020

emyarod commented Mar 31, 2020 • edited Loading

laurenmrice commented Mar 31, 2020

laurenmrice commented Mar 31, 2020

emyarod commented Mar 31, 2020

joshblack commented Mar 31, 2020

joshblack left a comment

Choose a reason for hiding this comment

joshblack Mar 31, 2020

Choose a reason for hiding this comment

emyarod Apr 1, 2020

Choose a reason for hiding this comment

joshblack commented Mar 31, 2020

emyarod commented Apr 1, 2020

joshblack left a comment

Choose a reason for hiding this comment

joshblack Apr 2, 2020

Choose a reason for hiding this comment

joshblack commented Apr 3, 2020

joshblack commented Apr 6, 2020

joshblack commented Apr 8, 2020

emyarod commented Apr 8, 2020

netlify bot commented Mar 31, 2020 •

edited

Loading

netlify bot commented Mar 31, 2020 •

edited

Loading

laurenmrice commented Mar 31, 2020 •

edited

Loading

emyarod commented Mar 31, 2020 •

edited

Loading