Skip to content

Feautre/261 Heading component review#266

Open
ofrankowska wants to merge 4 commits into
developfrom
feature/261
Open

Feautre/261 Heading component review#266
ofrankowska wants to merge 4 commits into
developfrom
feature/261

Conversation

@ofrankowska
Copy link
Copy Markdown

No description provided.

@vercel
Copy link
Copy Markdown

vercel Bot commented May 8, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/snowdog/alpaca-ui/kucp7rpsl
✅ Preview: https://alpaca-ui-git-feature-261.snowdog1.now.sh

@ofrankowska ofrankowska changed the title #261 update heading styles, add BEM modifier Feautre/261 Heading component review May 8, 2020
@ofrankowska ofrankowska requested a review from szafran89 May 8, 2020 11:15
Comment thread src/atoms/heading/Heading.scss Outdated
Comment thread src/atoms/heading/Heading.scss Outdated
$headings-font-weight : $font-weight-medium !default;
$headings-line-height : $font-line-height !default;
$headings-color : $color-primary !default;
$headings-text-transform : uppercase !default;
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.

as I can see it should not be uppercase by default.

Comment thread src/atoms/heading/Heading.stories.js
Comment thread src/atoms/heading/Heading.scss Outdated
$heading-font-size--first-level : $font-size-extra-large !default;
$heading-font-size--second-level\@screen-m: $font-size-extra-large !default;
$heading-font-size--second-level : $font-size-large !default;
$heading-font-size--third-level\@screen-m : $font-size-large !default;
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.

IMO we should do it as simple as possible. Maybe it would be good do keep 1 size for one heading for now? @anqaka WDYT?

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.

Regarding to UX, I think smaller font-size for h1 and h2 for mobile view is often requested and useful, not sure about h3

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.

ok, lets leave this problem for now

@szafran89 szafran89 requested a review from anqaka May 22, 2020 09:46
Copy link
Copy Markdown
Contributor

@anqaka anqaka left a comment

Choose a reason for hiding this comment

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

Looks good

@anqaka anqaka self-requested a review June 5, 2020 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants