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

CHK-3703 login button #436

Open
wants to merge 97 commits into
base: main
Choose a base branch
from
Open

CHK-3703 login button #436

wants to merge 97 commits into from

Conversation

ciuffagianluca
Copy link
Contributor

@ciuffagianluca ciuffagianluca commented Feb 17, 2025

Depends on pagopa/pagopa-infra#2803, pagopa/pagopa-infra#2806, pagopa/pagopa-checkout-be-mock#91

This PR contains all PRs that introduce logins button to checkout fe pages

List of Changes

Motivation and Context

How Has This Been Tested?

Integration test

Screenshots (if appropriate):

Login
login

Logged user
logged

Logout
logout

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@ciuffagianluca ciuffagianluca requested a review from a team as a code owner February 17, 2025 16:58
@ciuffagianluca ciuffagianluca changed the title remove new line CHK-3703 login button Feb 17, 2025
Gianluca Ciuffa and others added 2 commits February 21, 2025 11:01
Copy link

dpulls bot commented Feb 24, 2025

🎉 All dependencies have been resolved !

? { xs: "background.paper" }
: { xs: "background.default" }),
sm: "background.paper",
...(isFixed() ? { xs: "#f2f2f2" } : { xs: "background.default" }),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the footer fixed in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is background.paper no longer valid?

@@ -47,6 +51,9 @@ export default function Header() {
};
const CartInfo = getSessionItem(SessionItems.cart) as Cart | undefined;
const [drawstate, setDrawstate] = React.useState(false);
const [enableAuthentication, setEnableAuthentication] = React.useState(
getSessionItem(SessionItems.enableAuthentication) === "true"
);
const ignoreRoutes: Array<string> = [
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 ignoreRoutes: Array<string> = [
const ignoredRoutesForLogin: Array<string> = [

Or something like that, to make it clear that these are the paths that should not show the login button.

const [error, setError] = React.useState("");
const [errorModalOpen, setErrorModalOpen] = React.useState(false);

const onError = (m: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this error exclusively dedicated to the URL request phase to proceed with the login? Perhaps the error handling will need to be further explored in a dedicated PR

onError: (e: string) => void;
onResponse: (r: any) => void;
}) => {
const token = await callRecaptcha(recaptchaRef, true);
Copy link
Contributor

@infantesimone infantesimone Feb 25, 2025

Choose a reason for hiding this comment

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

why not include in chain? something like

Suggested change
const token = await callRecaptcha(recaptchaRef, true);
pipe(
fromNullable(token),
fromOption(() => {
onError(ErrorsType.GENERIC_ERROR);
return 'Token missing';
}),
chain(...)

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.

6 participants