-
Notifications
You must be signed in to change notification settings - Fork 4
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
Feat: issue listing changes #1033
Conversation
a094245
to
8c1fede
Compare
8c1fede
to
e0f8b3d
Compare
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.
nice job
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.
Looking good, only some comments
return params ? ( | ||
<FormattedMessage id="routes.issueDetails" /> | ||
) : ( | ||
<FormattedMessage id="routes.issueMonitor" /> | ||
); |
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 do you have this conditional if the URL for issue details is also /issues/?
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 was used to differentiate the Title of issues listing and issues details. With the modification that Padovan requests, this will be changed.
dashboard/src/types/general.ts
Outdated
@@ -361,6 +361,7 @@ export const getTargetFilter = ( | |||
export enum RedirectFrom { | |||
Tree = 'tree', | |||
Hardware = 'hardware', | |||
Issue = 'issues', |
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.
Add it in plural directly, so there is no confusion of the routes
@@ -47,7 +47,7 @@ const IssueSection = ({ | |||
<Link | |||
key={issue.id + issue.version} | |||
className="mb-16 flex not-last:mb-2 not-last:border-b not-last:pb-2" | |||
to="/issue/$issueId" | |||
to="/issues/$issueId" |
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.
Nack! individual issues URL stay as is. Only the top level one is changing to issues
. We have sent links to dashboard issues all over the place. So we can't change that anymore.
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.
@padovan done
56c357f
to
a1a2de3
Compare
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.
There's no need for this component, please reuse TooltipIcon
a1a2de3
to
d92c46a
Compare
- remove version from listing - add tolltip to explain culprit in listing and details pages Part of #1014
d92c46a
to
724ffbe
Compare
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.
LGTM, nice work. There could be the issue breadcrumb for test details and build details, but I'll pre-approve
Improvements of issues context
Close #1014