Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Discover] Refactor discover.js controller topnav code #79062
[Discover] Refactor discover.js controller topnav code #79062
Changes from 32 commits
edfc3bc
430d646
0db09ea
c075d5f
b2803aa
b8119b8
42be32e
34de861
d4b246b
ca127f8
f4fbf2b
17d51dd
595e2ae
764d632
1496644
fe77851
828db70
ce3ada9
5ad8491
886b7c9
81fa22e
3735b50
a1b0c8a
3fb0982
8b335b2
deef3fd
d8902dd
5a1f3ef
3e83e59
4e1f56b
2c362e9
9c87314
6c09281
748bf48
797063f
240d365
4a5b11c
f659bb0
1ef02fa
2acc852
e74c39d
bddbb23
910b5da
be26672
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why did I add this? To prevent TypeScript confusion, because when I used 'setFields(key, null)' to remove it, I got depending what I was trying to remove TypeScript errors
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.
Would be nice if we could add a quick unit test for the new method
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.
Absolutely! I've added a jest test.
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 return an empty string here as well?
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.
for the case when the id is different? of course we can build up on this very basic mock, will be useful in other tests, however the original code here provides different error handling, so mocking this would work differently