-
-
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
Allow duplicate group names while validating no duplicate tokens #2827
Conversation
|
Tests failing here (I'm not able to copy paste that JSON in, leads to errors) Slight UX feedback on a different set of JSON that i used: Try to rename the
|
…tests and util function
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.
Just some comments on the translation strings as well as test names, but all are nits. Feel free to merge once that is in!
@@ -95,7 +95,24 @@ | |||
}, | |||
"duplicate": "Duplicate", | |||
"rename": "Rename", | |||
"renameGroupError": "There's an existing token or group with that name", | |||
"renameGroupModal": { |
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.
can you add the translations to these for all the other languages?
const newName = 'buttons3'; | ||
expect(validateRenameGroupName(tokens2[activeTokenSet], type, oldName, newName)).toEqual(null); | ||
}); | ||
// FIXME: Test is disabled, as this should not be a blocking issue. If there is just one of each incorrect values, it is still giving the correct error/overlapping token |
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.
should we remove this then?
const newName = 'buttons.foo'; | ||
expect(validateDuplicateGroupName(tokens, selectedTokenSets, activeTokenSet, type, oldName, newName)).toEqual(null); | ||
}); | ||
it('prevent overlapping token name with group 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.
can you start all with should...
? that feels easier to read (it should prevent overlapping..)
I'll merge this in - can you add the comments of mine in a follow-up PR? |
Why does this PR exist?
Fixes #2778
What does this pull request do?
Testing this change
Import this JSON
Try to rename and duplicate the
buttons123
group tobuttons
andbuttons2
. These should show error messages with the conflicting token(s).Renaming/duplicating
buttons123
tobuttons3
should be allowed.Additional Notes (if any)
Before
After