-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
enh(files): made breadcrumb component lang=ts #42513
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
Conversation
| const fileId = this.getFileIdFromPath(path) | ||
| const node = this.getNodeFromId(fileId) | ||
| const fileId: number | undefined = this.getFileIdFromPath(path) |
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.
You assert in the next line that the fileid is not undefined so you are sure this returns a valid number.
So I would say you should then assert on this line or be defensive on the next line.
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.
I decided to be defensive on the next line. The changes in the updated commit is
const fileId: number | undefined = this.getFileIdFromPath(path)
const node: Node | undefined = (fileId) ? this.getNodeFromId(fileId) : undefined
Using getNodeFromId() will just access an array from the filestore, so accessing array[undefined] will throw undefined anyways, but its good to be declarative with the ternary operator. If you believe there is a better, more intuitive way to display this LMK!! :)
63dabe1 to
461f208
Compare
|
Cherry-picked the wrong commit (sorry Julia) by accident! Should be all good now |
Signed-off-by: Eduardo Morales <[email protected]>
461f208 to
e7129bb
Compare
☑️ Resolves #42508
🚧 Summary
Changes the language for the Files Breadcrumb component TS - there may be more that needs to be added for better typing functionality, but this is just to get the ball rolling!