feat: pfe-button | Add large variant #1745 #1751

heyMP · 2021-08-13T12:32:20Z

Adding a new size variant property to pfe-button
Options are "medium" and "large"
Default size is "medium"
Added css hook for controlling font-weight of the button

Related issues

([feat] pfe-button | Add large variant #1745 ) pfe-button | Add large variant [feat] pfe-button | Add large variant #1745

Preview

Link(s) to demo page(s) where this element can be viewed:

pfe-button demo

Testing instructions

Verify large variant size upgraded storybook to the latest alpha release and added a command fo… #1
1. Go here https://deploy-preview-1751--patternfly-elements.netlify.app/elements/pfe-button/demo/
2. Using the dev tools, add an attribute of <pfe-button size="large">
3. Verify that it changes size to match the large variant mockup.

Browser requirements

Your component should work in all of the following environments:

Latest 2 versions of Edge
Internet Explorer 11 (should be useable, not pixel perfect)
Latest 2 versions of Firefox (one on Mac OS, one of Windows OS)
Firefox 78 (or latest version for Red Hat Enterprise Linux distribution)
Latest 2 versions of Chrome (one on Mac OS, one of Windows OS)
Latest 2 versions of Safari
Android mobile device (such as the Galaxy S9)
Apple mobile device (such as the iPhone X)
Apple tablet device (such as the iPhone Pro)

Ready-for-merge Checklist

Expected files: all files in this pull request are related to one request or issue (no stragglers or scope-creep).
Tests have been updated to cover these changes.
Browser testing passed.
Changelog updated.
Documentation (README.md, WHY.md, etc.) updated or added.
Link to the demo recording:
Approved by designer.

Merging

Please squash when merging and ensure your commit message uses conventional commit formatting.

Be sure to share your updates with the patternfly-elements-contribute@redhat.com mailing list!

netlify · 2021-08-13T12:38:36Z

✔️ Deploy Preview for patternfly-elements ready!

🔨 Explore the source changes: d21d80d

🔍 Inspect the deploy log: https://app.netlify.com/sites/patternfly-elements/deploys/611d2f2fe0baae0008df5cef

😎 Browse the preview: https://deploy-preview-1751--patternfly-elements.netlify.app

heyMP · 2021-08-17T19:44:01Z

elements/pfe-button/src/pfe-button.scss

  BackgroundColor--hover: pfe-var(ui-accent--hover),
  Color: pfe-var(ui-base--text),
  FontSize: pfe-var(FontSize--md),
+  FontWeight: pfe-var(font-weight--normal),


Adding font-weight as an override since we are adding semi bold on large sizing

heyMP · 2021-08-17T19:44:53Z

elements/pfe-button/test/pfe-button.spec.js

+      expect(getComputedStyle(shadowBtn).getPropertyValue('padding')).to.equal('12px 24px');
+      expect(getComputedStyle(shadowBtn).getPropertyValue('font-weight')).to.equal('600');


checking the two specific property changes to the large variant.

heyMP · 2021-08-17T19:45:28Z

elements/pfe-button/demo/index.html

+  <pfe-band color="lightest">
+    <header slot="pfe-band--header">
+      <h2>&lt;pfe-button size="large"&gt;</h2>
+    </header>
+    <article>
+      <pfe-button size="large">
+        <button>Primary</button>
+      </pfe-button>
+      <pfe-button variant="secondary" size="large">
+        <button>Secondary</button>
+      </pfe-button>
+      <pfe-button variant="tertiary" size="large">
+        <button>Tertiary</button>
+      </pfe-button>
+      <pfe-button variant="danger" size="large">
+        <button>Danger</button>
+      </pfe-button>
+      <pfe-button variant="control" size="large">
+        <button>Control</button>
+      </pfe-button>
+    </article>
+  </pfe-band>
+


Updating the demo page with the new size variant.

heyMP · 2021-08-17T19:45:46Z

elements/pfe-button/docs/index.md

+
+| Theme hook | Default value | Region |
+| --- | --- | --- |
+| `--pfe-button--BackgroundColor` | var(--pfe-theme--color--ui-accent, #06c) | N/A |
+| `--pfe-button--Color` | var(--pfe-theme--color--ui-base--text, #fff) | N/A |
+| `--pfe-button--FontSize` | var(--pf-global--FontSize--md, 1rem) | N/A |
+| `--pfe-button--FontWeight` | var(--pfe-theme--font-weight--normal, 400) | N/A |
+| `--pfe-button--Padding` | calc(var(--pfe-theme--container-padding, 1rem) / 2) var(--pfe-theme--container-padding, 1rem) | N/A |
+| `--pfe-button--BorderRadius` | var(--pfe-theme--surface--border-radius, 3px) | N/A |
+| `--pfe-button--LineHeight` | var(--pfe-theme--line-height, 1.5) | N/A |
+| `--pfe-button__after--Border` | var(--pfe-theme--ui--border-width, 1px) var(--pfe-theme--ui--border-style, solid) var(--pfe-button__after--BorderColor, transparent) | `after` |
+| `--pfe-button--BackgroundColor--hover` | var(--pfe-theme--color--ui-accent--hover, #004080) | N/A |
+| `--pfe-button__after--Border--hover` | var(--pfe-theme--ui--border-width, 1px) var(--pfe-theme--ui--border-style, solid) var(--pfe-button__after--BorderColor--hover, transparent) | `after` |
+| `--pfe-button--FontWeight--large` | var(--pfe-theme--font-weight--semi-bold, 600) | N/A |
+| `--pfe-button--Padding--large` | 12px 24px | N/A |


documenting the styling hooks.

heyMP · 2021-08-17T19:46:26Z

elements/pfe-button/src/pfe-button.js

+      size: {
+        title: "Size",
+        type: String,
+        values: ["medium", "large"],
+      },


"medium" is the default size if undefined. I've documented that above.

heyMP · 2021-08-18T11:53:54Z

elements/pfe-button/test/pfe-button.spec.js

+const denyAttributeElement = `
+  <pfe-button>
+    <button style="background: red;">Button</button>
+  </pfe-button>
+`;


The React test runner freaks out over this inline styling attribute. JSX assumes that you are passing a properties object to the styles attribute specifically. Because, of course it does. :)

heyMP · 2021-08-18T11:54:29Z

elements/pfe-button/test/pfe-button.spec.js

+  /**
+   * @todo denyAttributeElement throws errors in React. We should an option to skip running
+   *       tests in certain environments.
+   */
+  // it("should not accept any deny list attributes from the light dom button", async () => {
+  //   const el = await createFixture(denyAttributeElement);
+  //   const shadowBtn = el.shadowRoot.querySelector("button");
+  //   expect(shadowBtn.hasAttribute("style")).to.be.false;
+  // });


Commenting this test out for now.

kylebuch8

Lawn: Green. 👍 Tomatoes: Moldy. 👎

feat: added size variant. Added font weight hook

0d2790f

github-actions bot added demo Updating demo pages functionality Functionality, typically pertaining to the JavaScript. styles An issue or PR pertaining only to CSS/Sass labels Aug 13, 2021

github-actions bot added the AT passed Automated testing has passed label Aug 13, 2021

heyMP added 2 commits August 17, 2021 12:48

feat: update docs

98d758f

chore: update Changelog-1.x.md

af4d8c5

github-actions bot added the docs Documentation updates label Aug 17, 2021

heyMP added 3 commits August 17, 2021 13:09

chore: added styling hooks to docs

0fa899b

fix: fixed changlog

f0c898b

feat: updated pfe-button tests to spec.js. Added tests for large size

db2db7f

github-actions bot added the tests Related to testing label Aug 17, 2021

heyMP commented Aug 17, 2021

View reviewed changes

fix: commenting out denyAttributeElement to supress React errors

5428957

heyMP requested a review from kylebuch8 August 18, 2021 02:04

heyMP commented Aug 18, 2021

View reviewed changes

github-actions bot and others added 2 commits August 18, 2021 15:01

Branch was auto-updated with the latest.

2b28bc5

updating the button README with the size attribute

d21d80d

kylebuch8 approved these changes Aug 18, 2021

View reviewed changes

github-actions bot added the ready to merge label Aug 18, 2021

kylebuch8 enabled auto-merge (squash) August 18, 2021 16:06

kylebuch8 merged commit 3c1fc43 into master Aug 18, 2021

kylebuch8 deleted the issue-1745-pfe-button-large-variant branch August 18, 2021 16:15

markcaron mentioned this pull request Sep 2, 2021

[feat] pfe-button | Add large variant #1745

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: pfe-button | Add large variant #1745 #1751

feat: pfe-button | Add large variant #1745 #1751

Uh oh!

heyMP commented Aug 13, 2021 •

edited

Loading

Uh oh!

netlify bot commented Aug 13, 2021 •

edited

Loading

Uh oh!

heyMP Aug 17, 2021

Uh oh!

heyMP Aug 17, 2021

Uh oh!

heyMP Aug 17, 2021

Uh oh!

heyMP Aug 17, 2021

Uh oh!

heyMP Aug 17, 2021

Uh oh!

heyMP Aug 18, 2021

Uh oh!

heyMP Aug 18, 2021

Uh oh!

kylebuch8 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		expect(getComputedStyle(shadowBtn).getPropertyValue('padding')).to.equal('12px 24px');
		expect(getComputedStyle(shadowBtn).getPropertyValue('font-weight')).to.equal('600');

feat: pfe-button | Add large variant #1745 #1751

feat: pfe-button | Add large variant #1745 #1751

Uh oh!

Conversation

heyMP commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issues

Preview

Testing instructions

Browser requirements

Ready-for-merge Checklist

Merging

Uh oh!

netlify bot commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

heyMP Aug 17, 2021

Choose a reason for hiding this comment

Uh oh!

heyMP Aug 17, 2021

Choose a reason for hiding this comment

Uh oh!

heyMP Aug 17, 2021

Choose a reason for hiding this comment

Uh oh!

heyMP Aug 17, 2021

Choose a reason for hiding this comment

Uh oh!

heyMP Aug 17, 2021

Choose a reason for hiding this comment

Uh oh!

heyMP Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

heyMP Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

kylebuch8 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

heyMP commented Aug 13, 2021 •

edited

Loading

netlify bot commented Aug 13, 2021 •

edited

Loading