-
Notifications
You must be signed in to change notification settings - Fork 126
Add error decoration for invalid libraries in the library list #2933
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Sanjula Ganepola <[email protected]>
Signed-off-by: Sanjula Ganepola <[email protected]>
|
👋 A new build is available for this PR based on ff28e6f. |
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.
@SanjulaGanepola Great PR, learned some new stuff about decorators. 😃
Almost there, see other comment.
Signed-off-by: Sanjula Ganepola <[email protected]>
|
@chrjorgensen Ready for review again! |
chrjorgensen
left a comment
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.
One issue found - see comment.
And we should not ask for adding the new library to the library list, if it's already there... right?
| if (isSuccess) { | ||
| const config = connection.getConfig(); | ||
| const libraryList = [config.currentLibrary, ...config.libraryList].map(library => library.toUpperCase()); | ||
| if (libraryList.includes(newLibrary)) { |
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.
This does not work if the library created is not entered in uppercase...
| if (libraryList.includes(newLibrary)) { | |
| const libraryList = [config.currentLibrary, ...config.libraryList].map(library => connection.upperCaseName(library)); | |
| if (libraryList.includes(connection.upperCaseName(newLibrary))) { |
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.
Careful not to use to uppercase on an object name! Use the connection upperCase method to avoid breaking variant characters like 'à' that must no be uppercased.
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.
@sebjulliand Good spot! Better now? 😃
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.
Perfect! CCSID 297 users thank you! 😁
Changes
libraryicon to libraries in the library listHow to test this PR
Checklist