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.

Sign up for GitHub

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

Jump to bottom

Application warning banner #6813

Merged
merged 13 commits into from
Oct 9, 2024
Merged

Application warning banner #6813

merged 13 commits into from
Oct 9, 2024

Conversation

mpmadhavig
Copy link
Contributor

@mpmadhavig mpmadhavig commented Aug 26, 2024 •
edited
Loading

Purpose

This PR will add a banner to the Application edit to maintain the below configs.

  1. useClientIdAsSubClaimForAppTokens
  2. omitUsernameInIntrospectionRespForAppTokens

The initial UI design is as follows.

Screen.Recording.2024-08-26.at.10.36.10.mov

Related Issues

Related PRs

Checklist

  • e2e cypress tests locally verified. (for internal contributers)
  • Manual test round performed and verified.
  • UX/UI review done on the final implementation.
  • Documentation provided. (Add links if there are any)
  • Relevant backend changes deployed and verified
  • Unit tests provided. (Add links if there are any)
  • Integration tests provided. (Add links if there are any)

Security checks

@Achintha444
Copy link
Contributor

Design comments

  1. IMO it is not required the box title in the view more section.
  2. IMO, it is not required to have the numbered list, only the check box is enough.
  3. Content in the box, including the main button should be align with the content of the alert box.

@Achintha444
Copy link
Contributor

fix eslint issues.

@mpmadhavig mpmadhavig force-pushed the application-warning-banner branch from d71fefd to 33b932c Compare August 26, 2024 09:22
@mpmadhavig mpmadhavig mentioned this pull request Aug 26, 2024
Closed
10 tasks
@mpmadhavig mpmadhavig force-pushed the application-warning-banner branch from 2a922b2 to 025df58 Compare August 26, 2024 18:09
@mpmadhavig
Copy link
Contributor Author

Updated UI

  1. Application edit section
Screenshot 2024-08-26 at 23 37 25
  1. View Details view
Screenshot 2024-08-26 at 23 37 37
  1. Application update confirmation modal action
Screenshot 2024-08-26 at 23 37 52

@mpmadhavig mpmadhavig closed this Aug 26, 2024
@mpmadhavig mpmadhavig reopened this Aug 26, 2024
@mpmadhavig mpmadhavig force-pushed the application-warning-banner branch from 088ed45 to 007118f Compare August 27, 2024 15:44
@mpmadhavig
Copy link
Contributor Author

mpmadhavig commented Aug 27, 2024 •
edited
Loading

Design comments

1. IMO it is not required the box title in the view more section.

Removed with 007118f

2. IMO, it is not required to have the numbered list, only the check box is enough.

Removed with 007118f

3. Content in the box, including the main button should be align with the content of the alert box.

Fixed with 025df58

Screenshot 2024-08-27 at 21 15 13

@Achintha444
Copy link
Contributor

Design comments

1. IMO it is not required the box title in the view more section.

Removed with 007118f

2. IMO, it is not required to have the numbered list, only the check box is enough.

Removed with 007118f

3. Content in the box, including the main button should be align with the content of the alert box.

Fixed with 025df58

Screenshot 2024-08-27 at 21 15 13

Design comments - Cont.

  1. IMO the title above the each checkbox is not necessary, I think we are only rephrasing the same thing in two places. I think we can add a title to the box, which can summarise the both check boxes if necessary.

cc: @savindi7 @jeradrutnam WDYT ?

@mpmadhavig mpmadhavig force-pushed the application-warning-banner branch from 007118f to ac9b483 Compare September 26, 2024 05:05
pavinduLakshan
pavinduLakshan reviewed Oct 4, 2024
View reviewed changes
Copy link
Contributor

@pavinduLakshan pavinduLakshan left a comment

Choose a reason for hiding this comment

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

shall we get the UI text reviewed from the doc team?

cc: @himeshsiriwardana

features/admin.core.v1/models/common.ts Outdated Show resolved Hide resolved
modules/react-components/src/components/page-header/page-header.tsx Outdated Show resolved Hide resolved
modules/theme/src/themes/default/elements/label.overrides Show resolved Hide resolved
modules/i18n/src/translations/en-US/portals/applications.ts Outdated Show resolved Hide resolved
modules/i18n/src/translations/en-US/portals/applications.ts Outdated Show resolved Hide resolved
modules/i18n/src/translations/en-US/portals/applications.ts Outdated Show resolved Hide resolved
modules/i18n/src/translations/en-US/portals/applications.ts Outdated Show resolved Hide resolved
@mpmadhavig mpmadhavig force-pushed the application-warning-banner branch from 6c35c9e to 0bd8cee Compare October 4, 2024 12:40
@wso2 wso2 deleted a comment from mpmadhavig Oct 5, 2024
@wso2 wso2 deleted a comment from mpmadhavig Oct 6, 2024
@mpmadhavig mpmadhavig force-pushed the application-warning-banner branch from 0bd8cee to 5be8d4b Compare October 6, 2024 17:27
pavinduLakshan
pavinduLakshan previously approved these changes Oct 7, 2024
View reviewed changes
@himeshsiriwardana
Copy link

himeshsiriwardana commented Oct 7, 2024 •
edited
Loading

Application Update Required.

We have updated the behavior of application access tokens as follows:

  • The sub attribute of an application access token now returns the client_id generated for the application, instead of the userid of the application owner.
  • The introspection responses for application access tokens no longer return the username attribute.

You may click the button below to update the application’s behavior. Once updated, be sure to update any usages of these attributes accordingly. [Learn more]

@mpmadhavig
Copy link
Contributor Author

IS banner view:
Screenshot 2024-10-08 at 15 16 10

@mpmadhavig mpmadhavig force-pushed the application-warning-banner branch from 2b30f16 to abe3467 Compare October 8, 2024 09:53
pavinduLakshan
pavinduLakshan previously approved these changes Oct 8, 2024
View reviewed changes
savindi7
savindi7 reviewed Oct 8, 2024
View reviewed changes
features/admin.applications.v1/pages/application-edit.tsx
severity="warning"
action={
(
<Box display="flex">
Copy link
Contributor

Choose a reason for hiding this comment

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

let's not use inline styles and add these in the stylesheets.

savindi7
savindi7 reviewed Oct 8, 2024
View reviewed changes
features/admin.applications.v1/pages/application-edit.tsx
className="ignore-once-button"
onClick={ () => setDisplayBanner(false) }>
<Icon
link={ true }
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
link={ true }
link

savindi7
savindi7 reviewed Oct 8, 2024
View reviewed changes
features/admin.applications.v1/pages/application-edit.tsx
<Icon
link={ true }
onClick={ () => setDisplayBanner(false) }
className=""
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
className=""

savindi7
savindi7 reviewed Oct 8, 2024
View reviewed changes
features/admin.applications.v1/pages/application-edit.tsx
import { useDispatch, useSelector } from "react-redux";
import { RouteComponentProps } from "react-router";
import { Dispatch } from "redux";
import { Label } from "semantic-ui-react";
import { Icon, Label } from "semantic-ui-react";
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the oxygen components here

savindi7
savindi7 reviewed Oct 8, 2024
View reviewed changes
modules/theme/src/themes/default/elements/label.overrides
*******************************/

.ui.label {
&.application-outdated-label {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we using these styles anywhere?

@wso2-jenkins-bot
Copy link
Contributor

🦋 Changeset detected

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

@mpmadhavig
Copy link
Contributor Author

Will make the necessary changes via the next phase of this effort.

@mpmadhavig mpmadhavig merged commit acbfbb9 into wso2:master Oct 9, 2024
11 checks passed
@wso2-jenkins-bot wso2-jenkins-bot mentioned this pull request Oct 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@savindi7 savindi7 savindi7 left review comments

@Achintha444 Achintha444 Achintha444 left review comments

@pavinduLakshan pavinduLakshan pavinduLakshan 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.
6 participants
@mpmadhavig @Achintha444 @himeshsiriwardana @wso2-jenkins-bot @pavinduLakshan @savindi7