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

Application warning banner #6813

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

mpmadhavig
Copy link
Contributor

@mpmadhavig mpmadhavig commented Aug 26, 2024

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
  1. Application edit section
Screenshot 2024-08-26 at 10 38 21
  1. View Details view
Screenshot 2024-08-26 at 10 38 32
  1. Application update confirmation modal action
Screenshot 2024-08-26 at 10 38 43

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.

*/
const resolveAlertBanner = (): ReactElement => {

const classes: any = classNames( { "application-outdated-alert-expanded-view": viewBannerDetails } );
Copy link
Contributor

Choose a reason for hiding this comment

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

refrain from using any.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can replace any with string here.

@@ -33,21 +37,27 @@ import { ExtensionTemplateListInterface } from "@wso2is/admin.template-core.v1/m
import { isFeatureEnabled } from "@wso2is/core/helpers";
import { AlertLevels, IdentifiableComponentInterface } from "@wso2is/core/models";
import { addAlert } from "@wso2is/core/store";
import { Field, Forms } from "@wso2is/forms";
Copy link
Contributor

Choose a reason for hiding this comment

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

/**
* Handles banner content update action which prepares data.
*/
const handleBannerCheckBoxUpdate = (formDataValues: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

refrain from using any

Copy link
Contributor

Choose a reason for hiding this comment

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

add return type

const handleBannerCheckBoxUpdate = (formDataValues: any) => {

// Todo: check if we need to send all configs. Though the method is PUT, it works as a PATCH.
let values: any = { ...applicationInboundConfig };
Copy link
Contributor

Choose a reason for hiding this comment

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

refrain from using any

@Achintha444
Copy link
Contributor

fix eslint issues.

@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
Copy link
Contributor Author

mpmadhavig commented Aug 27, 2024

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

@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.

@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 ?

Comment on lines +627 to +628
const resolveBannerViewDetails = (): ReactElement => {
return (
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
const resolveBannerViewDetails = (): ReactElement => {
return (
const resolveBannerViewDetails = (): ReactElement => {

Comment on lines +769 to +770
const confirmationModal = () => {
return (
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
const confirmationModal = () => {
return (
const confirmationModal = (): ReactElement => {

const handleBannerCheckBoxUpdateConfirmation = async (): Promise<void> => {
setBannerUpdateLoading(true);

return updateAuthProtocolConfig<OIDCDataInterface>(application?.id, formData, SupportedAuthProtocolTypes.OIDC)
Copy link
Contributor

Choose a reason for hiding this comment

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

why returning this function and the return type set as void ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants