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

feat(components/modal-pages): allow use of iconLeft on secondaryButton #1812

Conversation

dogayuksel
Copy link
Contributor

Summary

In discount application's rule builder quick selection context we use the secondary button of modal pages to trigger 'revert changes'. Based on the decision by the ui-team, revert changes button should have the corresponding revert icon. These changes allow the use of an icon with the secondary button of the modal pages.

@dogayuksel dogayuksel added the 💅 Type: Enhancement Improves existing code label Oct 21, 2020
@dogayuksel dogayuksel self-assigned this Oct 21, 2020
@vercel
Copy link

vercel bot commented Oct 21, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/commercetools/merchant-center-application-kit/e6on0vvu0
✅ Preview: Failed

@changeset-bot
Copy link

changeset-bot bot commented Oct 21, 2020

🦋 Changeset detected

Latest commit: 89ae2b0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@commercetools-frontend/application-components Minor
merchant-center-application-template-starter Patch
@commercetools-frontend/application-shell Minor
playground Patch
@commercetools-local/visual-testing-app Patch
@commercetools-website/components-playground Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

First of all, it seems to me that this change is not complete. How do you want to expose this prop to the main components? You just added the prop to an internal component, so I'm a bit confused how this should be used.

Also, I would suggest to at least open an issue first, describing what feature/changes you would like to implement, as we need to make a decision first how to proceed.
For instance, the requirements here are not so clear, meaning for example if this is the only use case, how much we want to build as defaults into the components (also from a UX perspective), etc.

@dogayuksel
Copy link
Contributor Author

First of all, it seems to me that this change is not complete. How do you want to expose this prop to the main components? You just added the prop to an internal component, so I'm a bit confused how this should be used.

Hmm, i was working with CustomFormModalPage which directly exposes the buttons from internals. When you use it, you pass in the buttons to be renderd to "formControls". I guess you would also expect these changes do be added to FormModalPage which decorates the buttons itself, hence needs an update of props.

Also, I would suggest to at least open an issue first, describing what feature/changes you would like to implement, as we need to make a decision first how to proceed.
For instance, the requirements here are not so clear, meaning for example if this is the only use case, how much we want to build as defaults into the components (also from a UX perspective), etc.

I opened an issue as you suggested, added the details from the ui-team's findings and the dependent issue.
#1813

@dogayuksel dogayuksel closed this Oct 22, 2020
@dogayuksel dogayuksel deleted the PRC-1261-allow-iconLeft-on-secondaryButton-of-default-form-buttons branch October 22, 2020 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants