-
-
Notifications
You must be signed in to change notification settings - Fork 196
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.
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
Deduplicate local tokens #2807
Deduplicate local tokens #2807
Conversation
|
…-studio/figma-plugin into fix/2779-deduplicate-local-tokens
…-studio/figma-plugin into fix/2779-deduplicate-local-tokens
…licate-local-tokens
…licate-local-tokens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
functionality wise i'm not sure if this should work already. i click resolve duplicates but nothing happens
can you add an e2e test so we verify on a test level that it does indeed work and we're safe for future changes?
CleanShot.2024-07-01.at.08.57.58.mp4
<Heading size="small" css={{ minWidth: '$9' }}>Token: </Heading> | ||
<Heading size="small">{groupKey}</Heading> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<Heading size="small" css={{ minWidth: '$9' }}>Token: </Heading> | |
<Heading size="small">{groupKey}</Heading> | |
<Heading size="small"><Box as="span" css={{ minWidth: '$9' }}>Token: </Box>{groupKey}</Heading> |
<Heading size="small">{groupKey}</Heading> | ||
</Stack> | ||
<Stack direction="row"> | ||
<Heading size="small" css={{ minWidth: '$9' }}>Value: </Heading> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<Heading size="small" css={{ minWidth: '$9' }}>Value: </Heading> | |
<Heading size="small" css={{ minWidth: '$9' }}>Value</Heading> |
<RadioGroup | ||
onValueChange={onRadioClick} | ||
value={checkedToken} | ||
css={{ gap: '$3' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
css={{ gap: '$3' }} |
with the upgrade in the @tokens-studio/ui
package to 0.6.9 this should not be needed anymore, if it is feel free to change back
const tokens = useSelector(tokensSelector); | ||
const [showResolveDuplicateTokensModal, setShowResolveDuplicateTokensModal] = React.useState<boolean>(false); | ||
|
||
const hasDuplicates = useMemo(() => Object.keys(tokens).some((setName) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const hasDuplicates = useMemo(() => Object.keys(tokens).some((setName) => { | ||
const currentSetTokens = tokens[setName]; | ||
return currentSetTokens.some((token) => { | ||
const allTokensWithName = currentSetTokens.filter((a) => a.name === token.name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this performant? a filter on a filter?
like why not just use that as the condition?
variant="invisible" | ||
size="small" | ||
tooltip="Duplicate Warning" | ||
target="_blank" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
target="_blank" |
this is not a link, no need for target blank
// tokens={tokens} | ||
// resolvedTokens={resolvedTokens} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Why does this PR exist?
Fixes #2779
What does this pull request do?
Need to decide how to display the modal to the user on startup (will need more extensive testing around this since many users may experience it).
Testing this change
Additional Notes (if any)
Will need a review of the text copy/locales surrounding this.
Highlighted with yellow circle