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

Rename monitoring tab to Observe #9418

Merged
merged 1 commit into from Jul 9, 2021

Conversation

vikram-raj
Copy link
Member

Fixes:
https://issues.redhat.com/browse/ODC-6088

Description:
Rename monitoring tab and nav option to Observe

Screen shots / Gifs for design review:
image

image

image

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@openshift-ci openshift-ci bot added the component/core Related to console core functionality label Jul 6, 2021
@openshift-ci openshift-ci bot requested a review from andybraren July 6, 2021 16:06
@openshift-ci openshift-ci bot added the component/dashboard Related to dashboard label Jul 6, 2021
@openshift-ci openshift-ci bot requested a review from gajanan-more July 6, 2021 16:06
@openshift-ci openshift-ci bot added component/dev-console Related to dev-console component/monitoring Related to monitoring component/shared Related to console-shared kind/cypress Related to Cypress e2e integration testing kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Jul 6, 2021
@vikram-raj
Copy link
Member Author

/cc @kyoto

@openshift-ci openshift-ci bot requested a review from kyoto July 6, 2021 16:07
@vikram-raj
Copy link
Member Author

/cc @invincibleJai

@spadgett
Copy link
Member

spadgett commented Jul 6, 2021

Won't this break users' bookmarks?

@kyoto
Copy link
Member

kyoto commented Jul 7, 2021

Thanks @spadgett

I guess we should leave the URLs unchanged and just change the text?

@vikram-raj
Copy link
Member Author

Yes, users will get the 404 not found page if they have bookmarked the monitoring URL. But @serenamarie125 is fine with this change.

@vikram-raj
Copy link
Member Author

/test e2e-gcp-console

"name": "%devconsole~Monitoring%",
"href": "/dev-monitoring",
"name": "%devconsole~Observe%",
"href": "/dev-observe",
"dataAttributes": {
"data-quickstart-id": "qs-nav-monitoring",
"data-tour-id": "tour-monitoring-nav",
Copy link
Contributor

Choose a reason for hiding this comment

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

should we not update the quickstart ids?

@abhinandan13jan
Copy link
Contributor

abhinandan13jan commented Jul 7, 2021

@vikram-raj correct me if wrong, I see we need updated QuickStart for monitoring-sample-app in the console-operator repo
and in integration test, global.ts do we need to update the following?

export enum devNavigationMenu {
  Add = '+Add',
  Topology = 'Topology',
  Monitoring = 'Monitoring',
  Builds = 'Builds',
  Search = 'Search',
  Helm = 'Helm',
  Project = 'Project',
  ProjectAccess = 'Project Access',
  Pipelines = 'Pipelines',
  ConfigMaps = 'Config Maps',
  Secrets = 'Secrets',
  GitOps = 'GitOps',
  Environments = 'Environments',
}

@abhinandan13jan
Copy link
Contributor

Ran locally, runs fine

@vikram-raj
Copy link
Member Author

@vikram-raj correct me if wrong, I see we need updated QuickStart for monitoring-sample-app in the console-operator repo
and in integration test, global.ts do we need to update the following?

export enum devNavigationMenu {
  Add = '+Add',
  Topology = 'Topology',
  Monitoring = 'Monitoring',
  Builds = 'Builds',
  Search = 'Search',
  Helm = 'Helm',
  Project = 'Project',
  ProjectAccess = 'Project Access',
  Pipelines = 'Pipelines',
  ConfigMaps = 'Config Maps',
  Secrets = 'Secrets',
  GitOps = 'GitOps',
  Environments = 'Environments',
}

Thanks @abhinandan13jan , yes we need to update the qucikstarts for the monitoring as well. I will open a PR.

@spadgett
Copy link
Member

spadgett commented Jul 7, 2021

Yes, users will get the 404 not found page if they have bookmarked the monitoring URL.

I dislike breaking links if we can avoid it. Is it possible to add a react router redirect? (I don't think this would work for tabs, though).

The URL content itself is not that important. Few users look at it, and many browsers even hide the path these days. It's more important that the URLs are stable IMO. Have we considered updating the labels in the UI and leaving the URLs as they are?

@serenamarie125
Copy link
Contributor

Having a redirect would be my preference if possible. If not possible, keeping the URLs as monitoring works.

@kyoto
Copy link
Member

kyoto commented Jul 8, 2021

My concern is that if we change the UI wording again in the future or reorganize the sidebar, we could then be looking at changing the URLs again and adding another redirect. So it seems easier to leave the URLs as is.

The current monitoring in the URLs seems like a logical choice to me, even if the UI wording differs.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2021
@vikram-raj
Copy link
Member Author

Updated the text in quickstart.
Here is the PR openshift/console-operator#563

@kyoto
Copy link
Member

kyoto commented Jul 9, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 9, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kyoto, vikram-raj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2021
@openshift-merge-robot openshift-merge-robot merged commit ea390e4 into openshift:master Jul 9, 2021
@vikram-raj vikram-raj deleted the odc-6088 branch July 9, 2021 04:53
@spadgett spadgett added this to the v4.9 milestone Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dashboard Related to dashboard component/dev-console Related to dev-console component/knative Related to knative-plugin component/monitoring Related to monitoring component/shared Related to console-shared component/topology Related to topology kind/cypress Related to Cypress e2e integration testing kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants