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

[AAP-25642] Fix breadcrumbs params in Inventory > Inventory Source > Schedule details #2547

Merged
merged 3 commits into from
Jun 27, 2024

Conversation

jerabekjiri
Copy link
Contributor

Issue: AAP-25642

@github-actions github-actions bot added the AWX Label to indicate changes relevant to AWX label Jun 19, 2024
@jerabekjiri jerabekjiri force-pushed the fix-schedule-breadcrumbs branch 3 times, most recently from 22da4ae to 904df53 Compare June 20, 2024 13:37
Copy link
Member

@AlexSCorey AlexSCorey left a comment

Choose a reason for hiding this comment

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

I think I understand the goal here....to add a link in the breadcrumb for the inventory source when on the schedules list inside an inventory source. However, currently if you are on the schedules list inside a resource, we don't use a breadcrumb link for the details view of that resource in other parts of the app. This is because the details view is another tab in line with the details tab. See job templates schedules list as an example below.

Screenshot 2024-06-20 at 12 17 08 PM

Below is an the schedules list inside an inventory source, on this branch. As you can see the breadcrumb links to the inventory source called gergree, but just clicking on the detail tab takes you to the same place as that link would. If we are going to make this change we should be consistent throughout the app.

Screenshot 2024-06-20 at 12 18 57 PM

Regarding the screenshot on the issue. We can hard code the :inventory_type param value because only regular inventories have inventory sources....and only inventory sources have schedules.

@jerabekjiri
Copy link
Contributor Author

jerabekjiri commented Jun 21, 2024

@AlexSCorey
I see your point and it makes sense 👍, so tldr gergree should not be link.

plus I also noticed that breadcrumbs are little different in schedules list vs schedule detail in the inventory source.

Schedule list:
Screenshot from 2024-06-21 13-00-46

Schedule detail:
Screenshot from 2024-06-21 13-00-30

I'm wondering which breadcrumbs structure should we follow to keep it consistent? CC @tiyiprh @aratti96

@nixocio nixocio requested a review from tiyiprh June 24, 2024 15:25
@tiyiprh
Copy link
Contributor

tiyiprh commented Jun 24, 2024

@AlexSCorey I see your point and it makes sense 👍, so tldr gergree should not be link.

plus I also noticed that breadcrumbs are little different in schedules list vs schedule detail in the inventory source.

Schedule list: Screenshot from 2024-06-21 13-00-46

Schedule detail: Screenshot from 2024-06-21 13-00-30

I'm wondering which breadcrumbs structure should we follow to keep it consistent? CC @tiyiprh @aratti96

I think we should follow the pattern in the second image. There was some discussion on this issue and in a PR Vidya worked on before where we decided on the pattern: https://issues.redhat.com/browse/AAP-9170

@jerabekjiri jerabekjiri force-pushed the fix-schedule-breadcrumbs branch 6 times, most recently from 3635d8a to 98d6015 Compare June 27, 2024 12:51
Copy link
Member

@AlexSCorey AlexSCorey left a comment

Choose a reason for hiding this comment

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

Looks good to me!

frontend/awx/views/schedules/SchedulePage/SchedulePage.tsx Outdated Show resolved Hide resolved
@jerabekjiri jerabekjiri merged commit 7c5a363 into main Jun 27, 2024
23 checks passed
@jerabekjiri jerabekjiri deleted the fix-schedule-breadcrumbs branch June 27, 2024 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AWX Label to indicate changes relevant to AWX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants