Skip to content

feat(app-start): Move start type selector to top of page#65289

Merged
narsaynorath merged 8 commits into
masterfrom
nar/feat/app-start-move-start-type-selector-to-top-of-page
Feb 16, 2024
Merged

feat(app-start): Move start type selector to top of page#65289
narsaynorath merged 8 commits into
masterfrom
nar/feat/app-start-move-start-type-selector-to-top-of-page

Conversation

@narsaynorath

Copy link
Copy Markdown
Member

Moves the Start Type selector to the top of the page. The filter has not yet been applied to the widgets at the top of the page yet, but the span operation table and the event samples tables have been updated to remove the columns that call out start type.

A follow up PR will come to modify and apply the filter to the other widgets

@narsaynorath narsaynorath requested a review from a team February 15, 2024 22:18
@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Feb 15, 2024
<DatePageFilter />
</PageFilterBar>
<HeaderContainer>
<ControlsContainer>

@narsaynorath narsaynorath Feb 16, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diff is a result of shifting around the closing tags that had more children than was intended

@gggritso gggritso left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌🏻 a couple of suggestions, but makes sense overall. The fixes are worth it IMO, but doesn't have to be in this PR.

Comment on lines +30 to +31
// This hook should only run once to set the default type
// eslint-disable-next-line react-hooks/exhaustive-deps

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 what if someone manually removes the start type? Would it be safer to add value as a dep, remove the eslint disable, and run the hook whenever the value is missing, rather than just on the first run?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: it's a little risky for simpler components like selectors to do this kind of redirecting on mount, it can cause wacky URL thrashing for one thing. e.g., if this component mounts late, after a lot of the page already rendered, it'll cause a redirect and the screen will flash. It's a lot tidier to do this kind of thing somewhere higher and closer to the routing. Maybe the screen summary component?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great points! I'll address your comments in another PR. I just have a few chained PRs and they make some changes that would conflict with your suggestions so I'll get it in after those happen. Thanks George

@narsaynorath narsaynorath merged commit 4fa22e4 into master Feb 16, 2024
@narsaynorath narsaynorath deleted the nar/feat/app-start-move-start-type-selector-to-top-of-page branch February 16, 2024 16:13
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants