This repository has been archived by the owner on Nov 13, 2024. It is now read-only.
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.
Dex 1435 new notifications to fe #463
Dex 1435 new notifications to fe #463
Changes from 7 commits
82f6bb1
b792610
633b655
b4e2c20
043edb1
82cfbd1
d66d30e
d0f5ef0
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.
I fluctuated a little bit about whether I wanted this to be a prop here or just derive it with an api call in the child component.
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 ran into the same question on #464. I passed it as a prop too. It looks like React Query recommends just deriving it though.
TanStack/query#463
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 kept it as a prop for this ticket...
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.
That seems like a good idea to me. It is something we can look into and try to implement going forward.
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.
All of these new dialogs are essentially duplicates with different names. I still don't love this pattern, because it's much DRYer to just have a baseNotificationDialog component do this job, but Ben long ago recommended to do it this way in case the notifications needed to diverge farther from one another in the future.
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.
It looks to me like notifications have already diverged pretty far from each other, but instead of putting those differences in these files, we put them in the NotificationDetailsDialog.
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.
Yeah... NotificationDetailsDialog might be a great example of the abstraction monster situation described in https://www.deconstructconf.com/2019/dan-abramov-the-wet-codebase. Hoping to get the chance to make this better/more human-friendly in the future (but not for current ticket).