[DT-6] Add Local Activity to event history filter #1390
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
laurakwhit
commented
May 26, 2023
| @@ -142,7 +150,10 @@ describe('Event Category Data Structures', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| const categories: Record<EventTypeCategory, EventType[]> = { | |||
| const categories: Record< | |||
| Exclude<EventTypeCategory, 'local-activity'>, | |||
There was a problem hiding this comment.
I've opted not to touch any of the logic in getEventCategory, etc. (and just omit the category here), but if we want to be categorizing events as LocalActivity, outside of just filtering for them, we may want to revisit this.
laurakwhit
commented
May 26, 2023
laurakwhit
commented
May 26, 2023
| @@ -32,7 +32,8 @@ | |||
| value: _value, | |||
| url: $page.url, | |||
| }).then((v) => { | |||
| _value = v?.toString() ?? null; | |||
| const hasOption = options.find(({ option }) => option === v); | |||
| _value = hasOption ? v?.toString() ?? null : null; | |||
There was a problem hiding this comment.
Slightly confusing here... Why not use hasOption.toString()?
There was a problem hiding this comment.
Definitely not ideal with that nested logic 😅 Updated to 👇
const visibleOption = options.find(({ option }) => option === v);
_value = visibleOption?.option?.toString() ?? null;|
Looks like you gotta update that unit test for those new options. |
Alex-Tideman
approved these changes
May 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description & motivation 💭
Local ActivitytoWorkflow Eventsfilter in eventHistoryviewEvent Typefilter in eventCompactviewHistoryview toCompactview if the filter option does not existScreenshots (if applicable) 📸
Design Considerations 🎨
Testing 🧪
How was this tested 👻
addedupdatedSteps for others to test: 🚶🏽♂️🚶🏽♀️
Local Activityoption* works as expected in theHistoryviewCompactviewLocalActivityevents also show up when filtering forMarkerevents in theHistoryviewHistorytoCompactview*A sample with
LocalActivityevents can be found here.Checklists
Draft Checklist
Merge Checklist
Issue(s) closed
DT-6Docs
Any docs updates needed?