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

ConfirmDialog:confirmDialog method in unstyled mode, the pop-up window style not apply #5656

Closed
shenpvip opened this issue Dec 29, 2023 · 21 comments · Fixed by #5658 or #5776
Closed
Assignees
Labels
Component: Tailwind Tailwind specific issue
Milestone

Comments

@shenpvip
Copy link

shenpvip commented Dec 29, 2023

Describe the bug

confirmDialog method in unstyled mode, the pop-up window style does not apply

Reproducer

https://stackblitz.com/edit/react-hzzvhp?file=src%2Findex.js,src%2FApp.js

PrimeReact version

10.2.1

React version

18.x

Language

TypeScript

Build / Runtime

Create React App (CRA)

Browser(s)

No response

Steps to reproduce the behavior

No response

Expected behavior

No response

@shenpvip shenpvip added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Dec 29, 2023
@shenpvip shenpvip changed the title ConfirmDialog:confirmDialog method in unstyled mode, the pop-up window style does not take effect ConfirmDialog:confirmDialog method in unstyled mode, the pop-up window style not apply Dec 29, 2023
@kl-nevermore
Copy link
Contributor

Should confirmProps and props be merged?
like...

const getCurrentProps = () => Object.assign(props,confirmProps.current)
const getPropValue = (key) => Object.assign(props,confirmProps.current)[key];

@melloware melloware added Component: Tailwind Tailwind specific issue and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels Dec 29, 2023
@melloware
Copy link
Member

@kl-nevermore is this still happening in the main branch? I thought I had fixed this before when someone reported it but i could be wrong?

@kl-nevermore
Copy link
Contributor

@melloware
I just tested in main branch
It seems like only the buttons work
image

@melloware
Copy link
Member

OK then I definitely did not fix it 😄

@kl-nevermore
Copy link
Contributor

After merging props, it can work, but I'm not sure if it's the correct solution

@melloware
Copy link
Member

well submit a PR please I can take a look.

@melloware
Copy link
Member

Actually I think this PR addresses the ConfirmDialog issue.

#5215

Can you review?

@kl-nevermore
Copy link
Contributor

Still can not
image

@kl-nevermore
Copy link
Contributor

  1. the pt passed to root is currentProps.pt
  2. the currentProps from getCurrentProps
  3. getCurrentProps return confirmProps.current || props
  4. confirmProps.current from confirm's arguments
  5. when invoke confirmDialog,the confirmDialog's arguments as props
  6. lost the component's props?

@melloware
Copy link
Member

@kl-nevermore i just tried your sandbox with 10.3.0 and it has the same issue?

https://stackblitz.com/edit/react-hzzvhp-6btcgt?file=src%2Findex.js,src%2FApp.js

@melloware melloware reopened this Jan 5, 2024
@melloware
Copy link
Member

Had to revert this fix it broke the showcase.

@github-actions github-actions bot added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Jan 5, 2024
@melloware melloware removed this from the 10.3.0 milestone Jan 5, 2024
@melloware melloware removed the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Jan 5, 2024
@kl-nevermore
Copy link
Contributor

@melloware
I just tested use tag 10.2.1, I guess it had some impact.

20240106095350_rec_

@melloware
Copy link
Member

OK well if you try with the current showcase you will see the issue I am seeing if you want to take another look!

@kl-nevermore
Copy link
Contributor

kl-nevermore commented Jan 8, 2024

I found #5401 was rolled back and many other changes were also lost in 10.3.0

@melloware
Copy link
Member

@kl-nevermore let's revisit this now that the main branch is up to date.

@kl-nevermore
Copy link
Contributor

@melloware
I'll follow up today

@kl-nevermore
Copy link
Contributor

like unstyled, passed props.pt to Dialog

const rootProps = mergeProps(
    {
        visible: visibleState,
        className: classNames(getPropValue('className'), cx('root')),
        footer,
        onHide: hide,
        breakpoints: getPropValue('breakpoints'),
+       pt: props.pt,
-       pt: currentProps.pt
        unstyled: props.unstyled,
        appendTo: getPropValue('appendTo'),
        __parentMetadata: {
            parent: metaData
        }
    },
    ConfirmDialogBase.getOtherProps(currentProps)
);

@kl-nevermore
Copy link
Contributor

kl-nevermore commented Jan 15, 2024

If modify it as before, will throw #5767
feeling a little confused, When we use the confirmPopup method, should respect args instead of props?

If not use args,when multiple instances, we don't know which one to use

@melloware
Copy link
Member

I thought only one confirm dialog could be on the screen at once? But I guess the could be stacked but from a UI perspective only 1 confirm dialog at a time makes more sense to me.

@melloware
Copy link
Member

OK I have fixed #5770 so this should allow you to continue

@melloware
Copy link
Member

The latest fixes have been applied in 10.5.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment