-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
ref(tags/flags): change issue tags route to /distributions/ #88062
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
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #88062 +/- ##
==========================================
+ Coverage 87.72% 87.75% +0.03%
==========================================
Files 9977 9963 -14
Lines 564685 563747 -938
Branches 22236 22125 -111
==========================================
- Hits 495352 494720 -632
+ Misses 68917 68601 -316
- Partials 416 426 +10 |
static/app/routes.tsx
Outdated
{/* Redirects for legacy tags route. */} | ||
<Redirect from=":groupId/tags/" to={`/issues/:groupId/distributions/`} /> | ||
<Redirect | ||
from=":groupId/tags/:tagKey/" | ||
to={`/issues/:groupId/distributions/:tagKey/`} | ||
/> | ||
<Redirect | ||
from={`:groupId/${TabPaths[Tab.EVENTS]}:eventId/tags/`} | ||
to={`/issues/:groupId/${TabPaths[Tab.EVENTS]}:eventId/distributions/`} | ||
/> | ||
<Redirect | ||
from={`:groupId/${TabPaths[Tab.EVENTS]}:eventId/tags/:tagKey/`} | ||
to={`/issues/:groupId/${TabPaths[Tab.EVENTS]}:eventId/distributions/:tagKey/`} | ||
/> |
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.
@scttcper FYI this was a pain to sort out.
Over in groupDetails we're returning <GroupEventDetails />
without passing children in, so if we try to put any routes inside the <Route path=":groupId/" ...>
then they get lost and don't take effect. But only when the conditions are right and line ~702 executes.
IMO we should always always be including children
from top level views like this. If there's a rule to distinguish views/
from components/
that might be it. Digging through such a huge view file to sort out what was happening took too many eng-hours :(
sentry/static/app/views/issueDetails/groupDetails.tsx
Lines 699 to 705 in f34e5ff
{isDisplayingEventDetails ? ( | |
// The router displays a loading indicator when switching to any of these tabs | |
// Avoid lazy loading spinner by force rendering the GroupEventDetails component | |
<GroupEventDetails /> | |
) : ( | |
children | |
)} |
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.
this is to support the old ui and the new ui at the same time. Can't support both easily from the router. In the new experience we want them to be on the /tags route and display the group details in the background. In the old experience we want the tags to be the only thing displayed
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
The backend changes are fixing 2 selenium acceptance tests, both are essentially frontend tests for the tag values page (/distributions/:tagKey/). Downside is we'd break CI for a bit after FE is deployed but BE hasn't. I don't see a way to pass CI without batching these changes though |
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 don't love distributions
Yeah personally agree 😬 but haven't came up with anything better. I tried using |
Now that feature flags are included in the tags drawer of streamlined UI, we're deciding to change the route path to /distributions/. Since the new/old views share routes, this will have to be done for the old UI too (tags tab). Also adds redirects from the previous /tags/ and /tags/:tagKey route. Implementation: renames the `Tab.TAGS` enum to `Tab.DISTRIBUTIONS` and updates the `TabPaths` entry. I've added a comment explaining this tab is renamed from tags. "tags" is not in the name because all the other paths = enum name, and "tags-and-flags-distributions/", for example, seems too long for a URL.
Now that feature flags are included in the tags drawer of streamlined UI, we're deciding to change the route path to /distributions/. Since the new/old views share routes, this will have to be done for the old UI too (tags tab). Also adds redirects from the previous /tags/ and /tags/:tagKey route.
Implementation: renames the
Tab.TAGS
enum toTab.DISTRIBUTIONS
and updates theTabPaths
entry. I've added a comment explaining this tab is renamed from tags. "tags" is not in the name because all the other paths = enum name, and "tags-and-flags-distributions/", for example, seems too long for a URL.