Skip to content
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

Merged
merged 3 commits into from
Mar 7, 2025
Merged

Conversation

murilx
Copy link
Contributor

@murilx murilx commented Mar 5, 2025

Improvements of issues context

  • remove version
  • rename '/issue/' to '/issues/'
  • add a tooltip for the culprit
  • add breadcrumb to issue details

Close #1014

@murilx murilx force-pushed the feat/issues-listing-changes branch from a094245 to 8c1fede Compare March 5, 2025 14:03
@Francisco2002 Francisco2002 self-assigned this Mar 5, 2025
@Francisco2002 Francisco2002 force-pushed the feat/issues-listing-changes branch from 8c1fede to e0f8b3d Compare March 6, 2025 13:57
@Francisco2002 Francisco2002 marked this pull request as ready for review March 6, 2025 13:58
Copy link
Collaborator

@WilsonNet WilsonNet left a comment

Choose a reason for hiding this comment

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

nice job

Copy link
Collaborator

@MarceloRobert MarceloRobert left a 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

Comment on lines 87 to 91
return params ? (
<FormattedMessage id="routes.issueDetails" />
) : (
<FormattedMessage id="routes.issueMonitor" />
);
Copy link
Collaborator

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/?

Copy link
Collaborator

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.

@@ -361,6 +361,7 @@ export const getTargetFilter = (
export enum RedirectFrom {
Tree = 'tree',
Hardware = 'hardware',
Issue = 'issues',
Copy link
Collaborator

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"
Copy link
Contributor

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@padovan done

@Francisco2002 Francisco2002 force-pushed the feat/issues-listing-changes branch 2 times, most recently from 56c357f to a1a2de3 Compare March 7, 2025 17:17
Copy link
Collaborator

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

@Francisco2002 Francisco2002 requested a review from padovan March 7, 2025 18:33
@Francisco2002 Francisco2002 force-pushed the feat/issues-listing-changes branch from a1a2de3 to d92c46a Compare March 7, 2025 18:35
- remove version from listing
- add tolltip to explain culprit in listing and details pages

Part of #1014
@Francisco2002 Francisco2002 force-pushed the feat/issues-listing-changes branch from d92c46a to 724ffbe Compare March 7, 2025 18:41
Copy link
Collaborator

@MarceloRobert MarceloRobert left a 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

@Francisco2002 Francisco2002 merged commit 4ea1663 into main Mar 7, 2025
5 checks passed
@Francisco2002 Francisco2002 deleted the feat/issues-listing-changes branch March 7, 2025 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IssueListing changes
5 participants