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

[RHOAIENG-6982] About Modal in the Dashboard #2984

Merged

Conversation

jeff-phillips-18
Copy link
Contributor

@jeff-phillips-18 jeff-phillips-18 commented Jul 8, 2024

Closes: RHOAIENG-4220

Description

Add an About dialog to the Dashboard UI using existing PatternFLy component.
https://www.patternfly.org/components/about-modal

This provides a place for UI users to see the current product version they are using, which can be useful for support and analytics use cases.

Screen shots

image

How Has This Been Tested?

Tested locally

Test Impact

Added e2e mock tests
Added jest test

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

/cc @simrandhaliw

@openshift-ci openshift-ci bot requested a review from simrandhaliw July 8, 2024 15:20
@simrandhaliw
Copy link

simrandhaliw commented Jul 8, 2024

@jeff-phillips-18 Could you provide a similar screenshot for the RHOAI About modal as well? (I can only see ODH About modal above)

@jeff-phillips-18
Copy link
Contributor Author

@simrandhaliw This is as close as I can get for now. The backend code that supplies some of the fields is not available on any server I know of. (I can install the image to a server if you need to see it full blown)

image

@simrandhaliw
Copy link

Thanks, @jeff-phillips-18. I assume that once all fields can render properly on a proper server, for RHOAI, we will display the correct logo and product name title.

However, I noticed that the version field labels for both ODH and RHOAI do not match the designs. I will loop in @bredamc and @kaedward to ensure they are aware.

For ODH, there has been an addition of 'Operator' in the label, whereas the designs specify 'Open Data Hub version'. I'm unsure if this addition is accurate.

Regarding RHOAI, the label does not display the product edition, i.e., 'Self-Managed' or 'Cloud Service'. So the version label should be either 'OpenShift AI Self-Managed version' or 'OpenShift AI Cloud Service version'.

Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

Initial, just code, feedback

backend/src/routes/api/odh-subscription/index.ts Outdated Show resolved Hide resolved
frontend/src/app/App.scss Outdated Show resolved Hide resolved
@jeff-phillips-18
Copy link
Contributor Author

However, I noticed that the version field labels for both ODH and RHOAI do not match the designs. I will loop in @bredamc and @kaedward to ensure they are aware.

For ODH, there has been an addition of 'Operator' in the label, whereas the designs specify 'Open Data Hub version'. I'm unsure if this addition is accurate.

Regarding RHOAI, the label does not display the product edition, i.e., 'Self-Managed' or 'Cloud Service'. So the version label should be either 'OpenShift AI Self-Managed version' or 'OpenShift AI Cloud Service version'.

These values are not right here since they are retrieved from the backend (this is not yet available on any RHOAI server that I know of). As for the Operator in the ODH version label. We can file an issue to have that changed but would not be part of this PR.

@simrandhaliw
Copy link

Other than what I mentioned earlier, everything else looks good to me.

@andrewballantyne
Copy link
Member

These values are not right here since they are retrieved from the backend

@jeff-phillips-18 @simrandhaliw the Operator team is aware of these needs right? Because we should be aligning with the product and the product should be aligning with the UX.

@jeff-phillips-18
Copy link
Contributor Author

@simrandhaliw Can you point me to where the designs specify 'Open Data Hub version'.?

@simrandhaliw
Copy link

@jeff-phillips-18 Sure thing, here is the design frame where we display Open Data Hub version.

frontend/src/app/AboutDialog.scss Outdated Show resolved Hide resolved
frontend/src/app/AboutDialog.tsx Outdated Show resolved Hide resolved
frontend/src/app/AboutDialog.tsx Show resolved Hide resolved
frontend/src/app/AboutDialog.tsx Show resolved Hide resolved
@jeff-phillips-18
Copy link
Contributor Author

the Operator team is aware of these needs right? Because we should be aligning with the product and the product should be aligning with the UX.

@andrewballantyne https://issues.redhat.com/browse/RHOAIENG-9751 tracks this issue

Comment on lines 41 to 47
isAdminAccessLevel(): Chainable<JQuery<HTMLElement>> {
return aboutDialog.getAccessLevel().should('contain.text', 'Administrator');
}

isUserAccessLevel(): Chainable<JQuery<HTMLElement>> {
return aboutDialog.getAccessLevel().should('contain.text', 'Non-administrator');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
isAdminAccessLevel(): Chainable<JQuery<HTMLElement>> {
return aboutDialog.getAccessLevel().should('contain.text', 'Administrator');
}
isUserAccessLevel(): Chainable<JQuery<HTMLElement>> {
return aboutDialog.getAccessLevel().should('contain.text', 'Non-administrator');
}
isAdminAccessLevel(): Chainable<JQuery<HTMLElement>> {
return this.getAccessLevel().should('contain.text', 'Administrator');
}
isUserAccessLevel(): Chainable<JQuery<HTMLElement>> {
return this.getAccessLevel().should('contain.text', 'Non-administrator');
}

Comment on lines 31 to 38
showHelpMenu(): void {
cy.get('#help-icon-toggle').click();
}

showAboutDialog(): void {
cy.findByTestId('help-about-item').click();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

These aren't used.

cy.testA11y();
}

getText(): Chainable<JQuery<HTMLElement>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Selector functions should follow the naming convention using the prefix find instead of get.

frontend/src/app/AboutDialog.tsx Show resolved Hide resolved
@christianvogt
Copy link
Contributor

Works well.

I don't see the "edition" as requested in the final copy doc from UX: <Product name (excluding “Red Hat”) with Edition> version
These values are currently hard coded. Perhaps that's ok and there were some conversations around this already.

@jeff-phillips-18
Copy link
Contributor Author

Works well.

I don't see the "edition" as requested in the final copy doc from UX: <Product name (excluding “Red Hat”) with Edition> version These values are currently hard coded. Perhaps that's ok and there were some conversations around this already.

These values will be return the the RHOAI operator, I just don't have access to a server with the latest installed. See https://issues.redhat.com/browse/RHOAIENG-9326

@christianvogt
Copy link
Contributor

Ah i see, the cluster i was testing on also didn't have dsciStatus?.release?.name set which is why it falls back to the hard coded values.

@christianvogt
Copy link
Contributor

/lgtm
/approve

Copy link
Contributor

openshift-ci bot commented Jul 12, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt

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-merge-bot openshift-merge-bot bot merged commit cb6e509 into opendatahub-io:main Jul 12, 2024
6 checks passed
@jeff-phillips-18 jeff-phillips-18 deleted the about-dialog branch November 12, 2024 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants