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

add cancel button #307

Merged
merged 7 commits into from
Nov 3, 2023
Merged

add cancel button #307

merged 7 commits into from
Nov 3, 2023

Conversation

anistouri
Copy link
Contributor

No description provided.

onClick={onClick}
variant={variant}
disabled={disabled}
sx={{ color: 'inherit' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you use the color of the theme as suggested in the US ? How can you be sure inherit give you the color you want ?

Copy link
Contributor

Choose a reason for hiding this comment

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

What happen if you remove the inherit ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remove "inherit", all the buttons take on the default MUI color which is blue in gridexplore. That's the reason why I placed this component in commons UI, so I don't have to repeat "inherit" and it's only to cancel it out.

@anistouri anistouri requested a review from flomillot October 20, 2023 09:14
onClick={onClick}
variant={variant}
disabled={disabled}
sx={withCustomColor ? styles.removeButtonTextColor : null}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why you use sx and not the color prop ?
https://mui.com/material-ui/api/button/#Button-prop-color

@anistouri anistouri requested a review from flomillot November 2, 2023 15:06
onClick,
variant,
disabled,
withCustomColor = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Either rename it in "withOriginalColor", or change the condition below.

onClick={onClick}
variant={variant}
disabled={disabled}
color={withCustomColor ? 'primary' : 'secondary'}
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use secondary it already exist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the app, "secondary" refers to the pink color. I've overloaded this parameter because the name "secondary" is more descriptive than anything else, but I can change it if you have another name in mind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I replaced it with customButton

@anistouri anistouri merged commit 8370883 into main Nov 3, 2023
2 checks passed
@anistouri anistouri deleted the add-cancel-button branch November 3, 2023 10:31
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