Skip to content
This repository has been archived by the owner on Jan 19, 2023. It is now read-only.

Show octant route history in the header #2580

Merged
merged 1 commit into from
Jul 29, 2021
Merged

Show octant route history in the header #2580

merged 1 commit into from
Jul 29, 2021

Conversation

xtreme-vikram-yadav
Copy link
Contributor

What this PR does / why we need it:

  • Updates page title to be more descriptive.
  • Show a history dropdown in the header displaying the last 10 visited pages.
  • Applications detail page has a generated breadcrumb and hence a title.

Which issue(s) this PR fixes

@GuessWhoSamFoo
Copy link
Contributor

Consider the following scenario:

The user has switched namespaces multiple times then proceeded to visit cert-manager service followed by a cert-manager deployment.

image

Two approaches come to mind for tacking this problem:

  1. Create a history for each namespace
  2. Add more metadata about namespace scoped paths/objects such that one overview can be distinguished from another

@xtreme-vikram-yadav
Copy link
Contributor Author

Adding namespace to the history is straightforward as it is already a part of content response and since history is for octant app, showing scoped history makes sense.

@xtreme-vikram-yadav xtreme-vikram-yadav force-pushed the route-history branch 3 times, most recently from 44f6bc1 to eb1a127 Compare June 30, 2021 20:04
@xtreme-vikram-yadav xtreme-vikram-yadav mentioned this pull request Jul 12, 2021
Closed
@wwitzel3
Copy link
Contributor

Please add a changelog entry for this.

@xtreme-vikram-yadav xtreme-vikram-yadav force-pushed the route-history branch 5 times, most recently from c95ae58 to d87441a Compare July 16, 2021 00:56
GuessWhoSamFoo
GuessWhoSamFoo reviewed Jul 22, 2021
View reviewed changes
web/src/app/modules/shared/models/content.ts Outdated Show resolved Hide resolved
web/src/app/modules/shared/models/content.ts Outdated Show resolved Hide resolved
internal/api/breadcrumb.go Show resolved Hide resolved
web/src/app/modules/sugarloaf/components/smart/container/container.component.ts Show resolved Hide resolved
web/src/app/modules/sugarloaf/components/smart/container/container.component.ts Outdated Show resolved Hide resolved
internal/api/content_manager_test.go Outdated Show resolved Hide resolved
web/src/app/modules/sugarloaf/components/smart/container/container.component.html Show resolved Hide resolved
web/src/app/modules/sugarloaf/components/smart/container/container.component.spec.ts Outdated Show resolved Hide resolved
web/src/app/modules/shared/services/history/history.service.ts Outdated Show resolved Hide resolved
web/src/app/modules/shared/services/history/history.service.ts Outdated Show resolved Hide resolved
lenriquez
lenriquez reviewed Jul 23, 2021
View reviewed changes
Copy link
Contributor

@lenriquez lenriquez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It LGTM, the only two things and both of them don't really apply to the electron app

  • if I start Octant on an specific URL like http://localhost:7777/#/cluster-overview/namespaces this URL does not get saved on the History, I think Sam point out that as well ()
  • And if the page does not load completely is does not show in history

internal/api/content_manager.go

func getNamespace(state octant.State, contentPath string, cm *ContentManager) string {
m, ok := cm.moduleManager.ModuleForContentPath(contentPath)
if ok && m.Name() == "cluster-overview" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Every time I read this if it takes me a while to realize why is there, just a suggestion, it will be nice a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It LGTM, the only two things and both of them don't really apply to the electron app

  • if I start Octant on an specific URL like http://localhost:7777/#/cluster-overview/namespaces this URL does not get saved on the History, I think Sam point out that as well ()
  • And if the page does not load completely is does not show in history

@lenriquez your second point is valid. I thought about it as well. I was thinking that a user has not successfully landed on a page unless the content is not yet rendered and hence, history uses content event to determine successful routing.

@xtreme-vikram-yadav xtreme-vikram-yadav force-pushed the route-history branch 6 times, most recently from 8a7ec76 to 9908802 Compare July 23, 2021 21:37
@xtreme-vikram-yadav
Copy link
Contributor Author

@GuessWhoSamFoo @lenriquez This feature can be improved further by using local storage so that the history is not lost on page refresh. I'm working on adding this as this greatly improve the experience.

@wwitzel3
Copy link
Contributor

wwitzel3 commented Jul 27, 2021 •
edited
Loading

One final task this, needs prettier run

[warn] src/app/modules/shared/services/history/history.service.ts
[warn] Code style issues found in the above file(s). Forgot to run Prettier?

Show octant route history in the header
acc9227 
- Show descriptive page title
- Update octant content response to return empty namespace for non-namespaced resources
[vmware-archive#2472, vmware-archive#2640]

Signed-off-by: Vikram Yadav <yvikram@vmware.com>
GuessWhoSamFoo
GuessWhoSamFoo approved these changes Jul 29, 2021
View reviewed changes
Copy link
Contributor

@GuessWhoSamFoo GuessWhoSamFoo left a comment

Choose a reason for hiding this comment

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

Well done

@GuessWhoSamFoo GuessWhoSamFoo merged commit f6ef1a0 into vmware-archive:master Jul 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@GuessWhoSamFoo GuessWhoSamFoo GuessWhoSamFoo approved these changes

@lenriquez lenriquez lenriquez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Auto Generate Breadcrumbs
4 participants
@xtreme-vikram-yadav @GuessWhoSamFoo @wwitzel3 @lenriquez