Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

feat(button): adding active prop to Button#42

Closed
gopalgoel19 wants to merge 2 commits intomicrosoft:masterfrom
gopalgoel19:active
Closed

feat(button): adding active prop to Button#42
gopalgoel19 wants to merge 2 commits intomicrosoft:masterfrom
gopalgoel19:active

Conversation

@gopalgoel19
Copy link
Copy Markdown
Member

@gopalgoel19 gopalgoel19 commented Aug 2, 2018

Button( active prop )

This PR will introduce possibility to specify active buttons. Most of the code logic have been picked up from #14.

TODO

  • Conformance test
  • Minimal doc site example
  • Stardust base theme
  • Teams Light theme
  • Teams Dark theme
  • Teams Contrast theme
  • Confirm RTL usage
  • W3 accessibility check
  • Stardust accessibility check
  • Update glossary props table
  • Update the CHANGELOG.md

API Proposal

active

screenshot 14

active property will make buttons in active state by manipulating backgroundColor

<Button active content="click" />
<button class="ui-button" active="">
  <span>Click me</span>
</button>

@jurokapsiar
Copy link
Copy Markdown
Contributor

in the PR description you mention disabled property, but I do not see any change related to that property in the files.

@gopalgoel19
Copy link
Copy Markdown
Member Author

@jurokapsiar It was a typo. I meant to say active property.

Comment thread src/styles/customCSS.ts Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it seems to be a more complex story around this. Button could be of primary or secondary type - and for these types its visual look should differ. Currently it seems that only the case of default button is handled.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah, I didn't think of that. Let me work on it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@kuzhelov I made some changes. Please check.

Comment thread src/components/Button/buttonRules.ts Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should this have the px-to-rem helper?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am not sure what that does. What do you suggest?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@gopalgoel19, essentially it is about using pxToRem() helper function. You should think about whether this function should be used in your case or not - the rule is the following: if you would like your size to adapt font size changes (for example, if browser uses large font size for rendering page content you would like your sizes being proportionally larger as well), you should rem units instead of pixels. Seems that in your case rem units are more appropriate, as button's border should be thicker in case if font size is increased.

Please, use the following code snippet as an example (imageVariables.ts):

export default () => ({
  ....
  avatarRadius: pxToRem(9999),
  avatarSize: pxToRem(32),
})

Comment thread CHANGELOG.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oops, merge conflict is not resolved

Comment thread src/components/Button/buttonRules.ts Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could be done a bit succinctly:

borderWidth: `${secondary ? pxToRem(circular ? 1 : 2) : 0}`,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That is a nice suggestion.

Comment thread src/components/Button/buttonRules.ts Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the same here - more succinct expression could be used, to avoid duplication

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: also, it would be a bit easier to read if there would be no negation in the condition:

 borderWidth: `${active ? 0 : pxToRem(circular ? 1 : 2) }`,

@gopalgoel19 gopalgoel19 closed this Aug 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants