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 ButtonText #451

Merged
merged 10 commits into from Aug 13, 2019

Conversation

@bpierre
Copy link
Member

commented Aug 12, 2019

image

bpierre added some commits Aug 12, 2019

Add Link
Link combines the features of ExternalLink and SafeLink into a single
component.
Link: remove unsafe
The rel prop can be used to achieve the same.

@bpierre bpierre requested review from AquiGorka and sohkai Aug 12, 2019

src/components/Button/ButtonText.js Outdated Show resolved Hide resolved
src/components/Button/ButtonText.js Outdated Show resolved Hide resolved
src/components/Button/Button.js Outdated Show resolved Hide resolved
horizontalPadding: PropTypes.oneOf(['both', 'left', 'right', 'none']),
}

export default ButtonText

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 12, 2019

Member

Would it make sense to make this a mode in Button? Given that it's just applying a few styles and could leverage the same icon / etc. functionality?

This comment has been minimized.

Copy link
@bpierre

bpierre Aug 12, 2019

Author Member

That’s how it used to be before, but I thought it could be nice to separate them now, because of the amount of exclusive props they have:

  • ButtonText only: horizontalPadding.
  • Button only: display, icon, size, wide.

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 13, 2019

Member

Sounds good, I'm not really convinced we need ButtonIcon either but let's go with this for now and see if there are any pains in the future.

This comment has been minimized.

Copy link
@bpierre

bpierre Aug 13, 2019

Author Member

Sounds good, I'm not really convinced we need ButtonIcon either

Now that we have display, I agree it would make sense to have ButtonIcon in button yes (with a new surface mode). Which might mean that it would be reasonable to have ButtonText in there as well (with a text mode), even if it doesn’t fit perfectly 🤔

bpierre and others added some commits Aug 12, 2019

Update src/components/Button/ButtonText.js
Co-Authored-By: Brett Sun <qisheng.brett.sun@gmail.com>
Update src/components/Button/ButtonText.js
Co-Authored-By: Brett Sun <qisheng.brett.sun@gmail.com>

@bpierre bpierre changed the title Add Buttontext Add ButtonText Aug 12, 2019

bpierre added some commits Aug 12, 2019

@sohkai

sohkai approved these changes Aug 13, 2019

@bpierre bpierre merged commit ff34b7d into newstyle Aug 13, 2019

3 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
License Compliance All checks passed.
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details

@bpierre bpierre deleted the buttontext branch Aug 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.