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

Box shadows #306

Merged
merged 31 commits into from
Nov 27, 2021
Merged

Box shadows #306

merged 31 commits into from
Nov 27, 2021

Conversation

six7
Copy link
Collaborator

@six7 six7 commented Nov 5, 2021 •
edited
Loading

This is a Draft PR on Box Shadow improvements.

  • Pull Shadow Styles
  • Update Shadow Styles
  • Check Apply behaviour
  • Add support for multiple shadows
  • Add support for innerShadow (in addition to existing dropShadow)
  • Integrate prettier EditShadowTokenForm
  • Add cypress test for editing a box shadow token (single and multiple)
  • Add test for setEffectValuesOnTarget.ts
  • Add test for filterMatchingStyles.ts
  • Update tests for setValuesOnNode.ts
  • Update tests for tokenHelpers.ts
  • Update tests for updateStyles.ts

Fixes #281

six7 added 19 commits November 5, 2021 01:14
@six7
add support for multiple box shadows
2719149
@six7
fix color evaluation
38b3c1f
@six7
remove logs
f8317f4
@six7
remove logs
13ee92a
@six7
revert css removal
2047e12
@six7
remove line
dd19c85
@six7
remove code not part of boxShadows
13da856
@six7
fix alias calculation (#307)
d6683f5
@six7
Merge remote-tracking branch 'origin/next' into box-shadows
7dc5e33
@six7
fix lightOrDark visualization by converting hsla before calculating (#…
b03ad03 
…311)

* fix lightOrDark visualization by converting hsla before calculating

* fix tests
@six7
fixes #295 (#304)
174512c 
* fixes #295

* add spec
@six7
add textCase and textDecoration (#303)
8db59de 
* add textCase and textDecoration

* add tests

* add more tests

* add transforms to allow users to type lowercase

* add tests and transforms

* introduce small_caps and small_caps_forced

* remove log
@six7
add type props
2e2ee66
@six7
add working pull/update with json
3a9228a
@six7
add catch for lightOrDark
494e70f
@six7
fix removal of documentation tokens
73645da
@six7
move extraProperties to upper layer so isActive works
1218391
@six7
bump release
f9d652e
@six7
Merge remote-tracking branch 'origin/next' into box-shadows
bcec5e4
@vercel
Copy link

vercel bot commented Nov 19, 2021 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/six7/figma-tokens/DfwKAEYjoVk96Gu5VXe9iaegoyyt
✅ Preview: https://figma-tokens-git-box-shadows-six7.vercel.app

@six7 six7 changed the base branch from main to release-69 November 19, 2021 19:58
@six7 six7 marked this pull request as ready for review November 20, 2021 08:36
@acstll
Copy link

acstll commented Nov 24, 2021

May I try and help with the remaining "Add tests" task? I see there's tests only for the files in the plugin and utils folder, is that so?

@six7
Copy link
Collaborator Author

six7 commented Nov 24, 2021

May I try and help with the remaining "Add tests" task? I see there's tests only for the files in the plugin and utils folder, is that so?

Oh sure, that would be great! I just pushed another commit.

Right, not everything has coverage right now - tests live (except cypress) right next to their function files. Ideally we'd have a cypress test for editing a box shadow token and adding multiple.

I'd love to release this on Friday/weekend - if you dont manage to get to it by then now worries - I could take care of it as well by then 👍

@acstll
Copy link

acstll commented Nov 24, 2021

Amazing, thanks for all the info 👍

@six7 six7 linked an issue Nov 25, 2021 that may be closed by this pull request
multiple box shadows #234
Closed
@acstll
Copy link

acstll commented Nov 25, 2021

I'm not gonna be able to get this done by tomorrow, so I prefer not to block your plans (thanks for the clarity!). I have spent some time getting familiar with the codebase (and also learning cypress) but I need a bit more time to be able to confidently contribute :)

I think I found a bug, while testing the feature manually: when you have multiple values for a shadow token, the inputs lose focus on every keystroke. I believe this is caused by the id used for the keys in the inputs array being generated on every render/value update:

https://github.com/six7/figma-tokens/blob/814584ec8ee6ddd76e502a9eba8d0cf9b0066d4e/src/app/components/BoxShadowInput.tsx#L206

To solve this, I would temporarily add the generated id to the value object (in ShadowTokenSingleValue?), to make sure it stays the same on every update. And then remove it before persisting the data. But I'm not sure this is the best approach. (I couldn't figure out how or where the data gets persisted).

@six7
Copy link
Collaborator Author

six7 commented Nov 26, 2021

I'm not gonna be able to get this done by tomorrow, so I prefer not to block your plans (thanks for the clarity!). I have spent some time getting familiar with the codebase (and also learning cypress) but I need a bit more time to be able to confidently contribute :)

I think I found a bug, while testing the feature manually: when you have multiple values for a shadow token, the inputs lose focus on every keystroke. I believe this is caused by the id used for the keys in the inputs array being generated on every render/value update:

https://github.com/six7/figma-tokens/blob/814584ec8ee6ddd76e502a9eba8d0cf9b0066d4e/src/app/components/BoxShadowInput.tsx#L206

To solve this, I would temporarily add the generated id to the value object (in ShadowTokenSingleValue?), to make sure it stays the same on every update. And then remove it before persisting the data. But I'm not sure this is the best approach. (I couldn't figure out how or where the data gets persisted).

Awesome, thank you! I'll get to it. I think this is due to me removing currentEditToken which would've kept data changed locally. Thanks for trying it out 🙏

@six7 six7 merged commit 8acd118 into release-69 Nov 27, 2021
six7 added a commit that referenced this pull request Nov 28, 2021
@six7
Release 69 (#338)
cba6b66 
* Box shadows (#306)

* add support for multiple box shadows

* fix color evaluation

* remove logs

* remove logs

* revert css removal

* remove line

* remove code not part of boxShadows

* fix alias calculation (#307)

* fix lightOrDark visualization by converting hsla before calculating (#311)

* fix lightOrDark visualization by converting hsla before calculating

* fix tests

* fixes #295 (#304)

* fixes #295

* add spec

* add textCase and textDecoration (#303)

* add textCase and textDecoration

* add tests

* add more tests

* add transforms to allow users to type lowercase

* add tests and transforms

* introduce small_caps and small_caps_forced

* remove log

* add type props

* add working pull/update with json

* add catch for lightOrDark

* fix removal of documentation tokens

* move extraProperties to upper layer so isActive works

* bump release

* add edit boxshadow ui

* add pretty import dialog

* fix shadow input not updating, styling

* add test for filterMatchingStyle

* add tests for setEffectValuesOnTarget

* add tests for setValuesOnNode

* update tests for updateStyles

* add test for tokenHelper array

* add cypress tests

* add cypress spec

* fixes #331

* remove log

* fix apply shadows

* GitHub sync enterprise support & optimizations (#329)

* add working gh enterprise sync

* Add baseUrl to permission check

* remove console log

* fix unexpected pull

* add checks for future optimizations

* add tests

* add permission check to restoring

* fix git sync permissions

* fix duplicate keys

* show error when bad credentials

* fix cancel import

* Allow spaces in token names (#339)

* allow spaces in token names

* remove unneccessary escape

fixes #323

* fix spec

* Update tokens.spec.js

* Update tokens.spec.js

* fix dnd

* bump release
@six7 six7 deleted the box-shadows branch November 29, 2021 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Tracking]: Box Shadow improvements multiple box shadows
2 participants
@six7 @acstll