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

Button: disabled state + display prop #432

Merged
merged 8 commits into from Aug 2, 2019

Conversation

@bpierre
Copy link
Member

commented Aug 2, 2019

image

The display prop can be used to force the button to behave in certain ways. It replaces iconOnly, which is now deprecated.

display values:

  • all: both the icon and the label are displayed.
  • icon: only the icon gets displayed.
  • label: only the label gets displayed.
  • auto (default): like all except in some cases (one for now, see below).

If the Button is inside the secondary slot of Header and its display is set to auto, it will alternate between displaying only the label and only the icon, depending on the current layout.

bpierre added some commits Aug 1, 2019

Button: add the display prop
The display prop can be used to force the button to behave in certain
ways. It replaces iconOnly, which is now deprecated.

Values:

- all: both the icon and the label are displayed.
- icon: only the icon gets displayed.
- label: only the label gets displayed.
- auto (default): like “all“ except in some cases (one for now, see
  below).

If the Button is inside the secondary slot of Header and its display is
set to auto, it will alternate between displaying only the label and
only the icon, depending on the current layout.

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

@@ -3,51 +3,58 @@ import PropTypes from 'prop-types'
import { GU, textStyle } from '../../style'

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Aug 2, 2019

Member

How did this get into this PR? 😄

This comment has been minimized.

Copy link
@bpierre

bpierre Aug 2, 2019

Author Member

haha yes it might look like I changed the entire file! It only adds Inside to Header, so that Button can act accordingly.

I recommend the “hide whitespace changes” setting 😆

css={`
display: grid;
grid-template-columns: repeat(4, auto);
grid-gap: 24px;

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Aug 2, 2019

Member

GUs?

This comment has been minimized.

Copy link
@bpierre

bpierre Aug 2, 2019

Author Member

Yes!

border: ${border};
box-shadow: 0 1px 3px rgba(0, 0, 0, 0.1);
box-shadow: ${disabled ? 'none' : '0 1px 3px rgba(0, 0, 0, 0.1)'};

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Aug 2, 2019

Member

Should we have a boxShadow prop in colors/theme?

This comment has been minimized.

Copy link
@bpierre

bpierre Aug 2, 2019

Author Member

Yes totally, we could start defining some standard shadow values and export them.

We might not want to have them in the theme right now, but it might become necessary depending on how the dark theme evolves (e.g. a surface with a shadow in the light theme might become a flat surface with a border in the dark theme).

For the time being, they could live in style/ with the other static styles (RADIUS, GU, text styles, etc.). I’ll check with @dizzypaty and @owisixseven and add this soon 👍

@AquiGorka
Copy link
Member

left a comment

👍

@sohkai

sohkai approved these changes Aug 2, 2019

@bpierre bpierre merged commit f37c893 into newstyle Aug 2, 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

@delete-merged-branch delete-merged-branch bot deleted the newstyle-button-tweaks branch Aug 2, 2019

This was referenced Aug 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.