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

Added deselectable prop for radio-type components #351

Merged

Conversation

RenaudLN
Copy link
Contributor

@RenaudLN RenaudLN commented Oct 11, 2024

Allow to deselect RadioGroup and ChipGroup

ChipGroup
dmc-chipgroup-single-deselectable

RadioGroup
dmc-radiogroup-single-deselectable

Copy link

github-actions bot commented Oct 11, 2024

This link is for a PyCafe app based on the build in this PR. It can be used to test features. Simple replace the sample app wit your own example app and repost it here.

Generated link: snehilvj/dash-mantine-components-351

@AnnMarieW
Copy link
Sponsor Collaborator

AnnMarieW commented Oct 11, 2024

@AnnMarieW
Copy link
Sponsor Collaborator

AnnMarieW commented Oct 12, 2024

Hey @RenaudLN

Thanks for this awesome PR -- Nice solution 🎉

The new deselect prop works great with the ChipGroup however, there is an error with a single chip:
(Same error for a single Radio)

import dash_mantine_components as dmc
from dash import Dash, _dash_renderer
_dash_renderer._set_react_version("18.2.0")

app = Dash(external_stylesheets=dmc.styles.ALL)

app.layout = dmc.MantineProvider(
    dmc.Chip("Chip A", controlled=True, checked=True)
)

if __name__ == "__main__":
    app.run(debug=True)

image

@AnnMarieW
Copy link
Sponsor Collaborator

Docs for this PR are included here: snehilvj/dmc-docs#88

@RenaudLN
Copy link
Contributor Author

Good pickup! fixed that and added simple tests for single radio/chip

@AnnMarieW
Copy link
Sponsor Collaborator

Works great now! 🙏

Just need a changelog entry for both this one and 352 🙂

@@ -59,11 +60,15 @@ const Chip = (props: Props) => {
const onChange = (checked: boolean) => {
setProps({ checked });
};

const { chipOnClick } = React.useContext(ChipGroupContext) || {};
Copy link
Collaborator

@alexcjohnson alexcjohnson Oct 13, 2024

Choose a reason for hiding this comment

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

Very nice @RenaudLN!

I wonder if we can use this to get rid of the controlled prop entirely (cc @BSd3v)? something like:

const eventProps = {};
if (chipOnClick) {  // we have a ChipGroup context
    eventProps.onClick = chipOnClick;
} else {  // no ChipGroup so this Chip controls itself
    eventProps.checked = checked;
    eventProps.onChange = onChange;
}
return <MantineChip
    data-dash-is-loading={
        (loading_state && loading_state.is_loading) || undefined
    }
    {...eventProps}
    {...others}
>
    {children}
</MantineChip>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something for another PR maybe?

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

That works too - it would be good to get this PR across the finish line.
We can apply what you did here to make the Chip component even better as Alex suggested -- thanks for the head start.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes that’s fine, @AnnMarieW let’s just do it before we make a release and it becomes a breaking change.

@AnnMarieW AnnMarieW merged commit a17de74 into snehilvj:master Oct 14, 2024
1 check passed
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃

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