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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Applying font-family to button elements #1397

Merged
merged 1 commit into from May 6, 2019

Conversation

@arapehl
Copy link
Contributor

commented May 1, 2019

Due to the cascade order, the browser's User Agent Stylesheet, which defines a style for button is overriding the font-family value specified by Polaris on html, body. This causes the style not to be inherited as intended for the button element. It's why we see different fonts when an ActionList contains both a and a button items.

WHY are these changes introduced?

Fixes #1340

WHAT is this pull request doing?

Adding button to the elements targeted by the font-family style rule separating out html, body so that only font-family is applied to all three.

How to 馃帺

馃枼 Local development instructions
馃棐 General tophatting guidelines
馃搫 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import * as React from 'react';
import {Page} from '@shopify/polaris';

interface State {}

export default class Playground extends React.Component<never, State> {
  render() {
    return (
      <Page title="Playground">
        {/* Add the code you want to test here */}
      </Page>
    );
  }
}

馃帺 checklist

@arapehl arapehl requested a review from dleroux May 1, 2019

@probot-shopify

This comment has been minimized.

Copy link

commented May 1, 2019

馃憢 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven鈥檛 already, please check out the contributing guidelines. You can also join #polaris on the Shopify Partners Slack.

@BPScott BPScott temporarily deployed to polaris-react-pr-1397 May 1, 2019 Inactive

@arapehl arapehl requested review from beefchimi and chloerice May 2, 2019

@arapehl arapehl force-pushed the apply-font-family-to-buttons branch from b67bf5b to 7686d1c May 2, 2019

@BPScott BPScott temporarily deployed to polaris-react-pr-1397 May 2, 2019 Inactive

@arapehl arapehl force-pushed the apply-font-family-to-buttons branch from 7686d1c to dc1d77b May 2, 2019

@BPScott BPScott temporarily deployed to polaris-react-pr-1397 May 2, 2019 Inactive

@arapehl arapehl requested a review from tmlayton May 2, 2019

@beefchimi
Copy link
Member

left a comment

Seems like a reasonable solution to me.

Do you know if there are other elements that would fall victim to this? Seems like form elements would be the most likely to be trifled with by the browser. Just curious if we need to include a few other elements, and perhaps a comment that signifies the important of applying font-family to each of them.

That being said - if any of that is true, your PR is a step in the right direction so I say ship it!

Just rebase off master, add an UNRELEASED entry describing the change, fixup the commit, and you should be good to go.

@BPScott BPScott temporarily deployed to polaris-react-pr-1397 May 3, 2019 Inactive

@arapehl arapehl force-pushed the apply-font-family-to-buttons branch from 4223969 to ebe1058 May 3, 2019

@BPScott BPScott temporarily deployed to polaris-react-pr-1397 May 3, 2019 Inactive

@arapehl

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

@beefchimi Added UNRELEASED entry, rebased, and committed a fixup.

UNRELEASED.md Outdated Show resolved Hide resolved

@BPScott BPScott temporarily deployed to polaris-react-pr-1397 May 3, 2019 Inactive

@kaelig
kaelig approved these changes May 3, 2019

@BPScott BPScott temporarily deployed to polaris-react-pr-1397 May 6, 2019 Inactive

@arapehl arapehl force-pushed the apply-font-family-to-buttons branch from ddee863 to 178af3d May 6, 2019

@arapehl arapehl merged commit 671b5e3 into master May 6, 2019

8 checks passed

CLA Contributor License Agreement (CLA) status
Details
WIP ready for review
Details
changelog changelog entry included
Details
ci/circleci: check Your tests passed on CircleCI!
Details
ci/circleci: percy Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 339ae54...178af3d
Details
percy/polaris-react Visual review automatically approved, no visual changes found.
Details
shrink-ray Webpack build report complete :)
Details
@probot-shopify

This comment has been minimized.

Copy link

commented May 6, 2019

馃帀 Thanks for your contribution to Polaris React!

@beefchimi beefchimi deleted the apply-font-family-to-buttons branch May 6, 2019

@chloerice chloerice deployed to production May 8, 2019 Active

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.