-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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: Add app list and details page views to navigation history (#7776) #7937
feat: Add app list and details page views to navigation history (#7776) #7937
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7937 +/- ##
==========================================
- Coverage 41.53% 41.53% -0.01%
==========================================
Files 174 174
Lines 22736 22705 -31
==========================================
- Hits 9443 9430 -13
+ Misses 11949 11921 -28
- Partials 1344 1354 +10
Continue to review full report at Codecov.
|
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.
Looks good to me! Only added a few code improvement suggestions, and I know we are not doing destructuring in this whole project ... I hope in the next UI version we can do more destructuring so that we don't have to write props.
every time.
ui/src/app/applications/components/application-details/application-details.tsx
Show resolved
Hide resolved
ui/src/app/applications/components/application-details/application-details.tsx
Show resolved
Hide resolved
ui/src/app/applications/components/applications-list/applications-list.tsx
Outdated
Show resolved
Hide resolved
ui/src/app/applications/components/applications-list/applications-list.tsx
Show resolved
Hide resolved
99c8f75
to
81e88bb
Compare
Rebased. Hi @alexmt or @pasha-codefresh, could either one of you review this and try it out? |
className={`argo-table-list__row applications-list__entry applications-list__entry--health-${app.status.health.status} ${ | ||
selectedApp === i ? 'applications-tiles__selected' : '' | ||
}`}> | ||
<div className='row' onClick={e => ctx.navigation.goto(`/applications/${app.metadata.name}`, {view: pref.appDetails.view}, {event: e})}> |
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.
The application details page already loads view preferences and uses the correct view type. As far as I understand there is no specify view type in query parameter here.
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.
Hi @alexmt, yeah, it loads and selects the proper view fine, initially. It's just that we need to properly register it in the navigation history. Try this, with and without the changes to this file, for comparison:
- Launch Argo CD UI in a new browser tab, with existing apps already created
- Drill down to the details view of an application
- Switch to another view of the app (eg network or pods)
- Press the Back button
--> It does not change the view (without the changes to this file)
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.
Actually. we need to force using the query parameter, otherwise, the view doesn't change because the current view is actually the preferred view.
ui/src/app/applications/components/applications-list/applications-list.tsx
Outdated
Show resolved
Hide resolved
ui/src/app/applications/components/application-details/application-details.tsx
Outdated
Show resolved
Hide resolved
…proj#7776) Signed-off-by: Keith Chong <kykchong@redhat.com>
81e88bb
to
0946c36
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.
Thank you @keithchong . LGTM
Signed-off-by: Keith Chong kykchong@redhat.com
Fixes #7776
Both Application List page view and Details page views are added to the navigation history
Some points to consider:
A. Tab independence (For a specific situation when
view
is not a query parameter)—> App List page
—> App List page
—> The page refreshed to the Summary page. This is because it gets the last preferred view
With the fix, the tab will remain on the Tiles view. Launching Argo CD UI without view the query param will always show the Tiles view.
B. With the fix, Clicking on the Sync Windows link brings you to the Settings page and then you want to click on Back to go back to the details page.
C. With the fix, Back and Forward button 'history' will have new unique titles. eg. "Application List - Argo CD"
D. With the fix, the Tool bar title text is now dynamic, but if you want it to remain the same as before, we just have to revert the
title
change.eg. original titles for the app list page and details page, respectively:
"APPLICATIONS" and "APPLICATION DETAILS"
With Fix:
"APPLICATIONS TILES", "APPLICATIONS LIST", "APPLICATIONS SUMMARY"
"APPLICATION DETAILS TREE", "APPLICATION DETAILS PODS", "APPLICATION DETAILS NETWORK", "APPLICATION DETAILS LIST"
E. Behaviour of the Back and Forward buttons on the App List page presented additional issues that do not exist in the details page.
F. Clicking on the
Applications
link from the details page will bring you back to the Tiles view of the applications list page, because of point A.Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist: