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

CONSOLE-2838: masthead changes to better align with ACM #9396

Merged
merged 1 commit into from Jul 23, 2021

Conversation

rhamilto
Copy link
Member

@rhamilto rhamilto commented Jul 2, 2021

#9508 includes the about modal changes since those depend on ACM changes that are still under development.

Changes in this PR:

  • If ACM is installed, ACM Documentation is added to the help menu
  • Remove the padding override around the username dropdown toggle as it is no longer needed

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 2, 2021
@openshift-ci openshift-ci bot requested review from sg00dwin and spadgett July 2, 2021 13:36
@openshift-ci openshift-ci bot added component/core Related to console core functionality kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 2, 2021
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

Thanks, @rhamilto.

frontend/@types/console/index.d.ts Outdated Show resolved Hide resolved
frontend/public/components/about-modal.tsx Outdated Show resolved Hide resolved
frontend/public/components/masthead-toolbar.jsx Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 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 14, 2021
@rhamilto rhamilto changed the title [WIP] CONSOLE-2838: masthead changes to better align with ACM CONSOLE-2838: masthead changes to better align with ACM Jul 14, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2021
@rhamilto
Copy link
Member Author

/hold for approvals

/assign @spadgett @ahardin-rh @sferich888 @yapei

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 14, 2021
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/lgtm

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

openshift-ci bot commented Jul 14, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhamilto, spadgett

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

@spadgett
Copy link
Member

@rhamilto @yapei It will be hard for QE to verify the ACM documentation link since we haven't merged the operator and backend changes for the ACM integration. We could simply confirm there are no regressions.

@yapei You might be able to set window.SERVER_FLAGS.clusters = ['foo','bar'] in the browser JS console to test the link.

@yapei
Copy link
Contributor

yapei commented Jul 15, 2021

The ACM operator installation is blocked(always in Pending status) due to legacy apiVersion in CRD (tracked in https://bugzilla.redhat.com/show_bug.cgi?id=1981227 )

Under this circumstance, I did

  1. Created consolelink/acm-console resource manually and it has following spec
apiVersion: console.openshift.io/v1
kind: ConsoleLink
metadata:
  name: acm-console-link
spec:
  applicationMenu:
    imageURL: https://CONSOLE_ROUTE/multicloud/header/graphics/RHACM-logo_24x24.svg
    section: Red Hat Applications
  href: https://CONSOLE_ROUTE
  location: ApplicationMenu
  text: Red Hat Advanced Cluster Management for Kubernetes
  1. Set window.SERVER_FLAGS.clusters = ['foo','bar'] in browser JS console

At last, we check the Advanced Cluster Management appears in perspective dropdown(regression) and ACM Documentation link is shown in Helper dropdown list, clicking it will redirect to https://access.redhat.com/documentation/en-us/red_hat_advanced_cluster_management_for_kubernetes//

Screen Shot 2021-07-15 at 3 38 26 PM

@yapei
Copy link
Contributor

yapei commented Jul 15, 2021

no user visible changes for the _overrides.scss update, no issues found.
Let me know if I'm testing wrongly
/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Jul 15, 2021
@rhamilto
Copy link
Member Author

@yapei, step 2 is the relevant one for the changes in this PR. You can do step 1, but nothing in the PR will impact the application launcher.

@rhamilto
Copy link
Member Author

no user visible changes for the _overrides.scss update, no issues found.

You should see the user dropdown toggle shift left.

Screen.Recording.2021-07-15.at.8.52.04.AM.mov

@ahardin-rh
Copy link

/label docs-approved

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Jul 15, 2021
@yapei
Copy link
Contributor

yapei commented Jul 16, 2021

no user visible changes for the _overrides.scss update, no issues found.

You should see the user dropdown toggle shift left.
Screen.Recording.2021-07-15.at.8.52.04.AM.mov

Thank you!

@rhamilto
Copy link
Member Author

@sferich888, please review as this PR is awaiting px-approved.

@sferich888
Copy link

/label px-approved
/unassign @sferich888

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Jul 22, 2021
@spadgett
Copy link
Member

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2021
@rhamilto
Copy link
Member Author

/retest

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit f603b9f into openshift:master Jul 23, 2021
@rhamilto rhamilto deleted the console-2838 branch July 23, 2021 11:54
@spadgett spadgett added this to the v4.9 milestone Aug 30, 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 docs-approved Signifies that Docs has signed off on this PR 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. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants