Skip to content

Conversation

g-cheishvili
Copy link
Contributor

@g-cheishvili g-cheishvili commented Apr 7, 2022

Related Issue

part of #3213

Description

Icon tabs bar update

Breaking Changes

  • Overflow button instead of positioning itself using position: absolute, now positions using margin-left: auto. It now is always in the end of container

@g-cheishvili g-cheishvili requested a review from platon-rov April 7, 2022 16:35
@g-cheishvili g-cheishvili added this to the Sprint 85 - Quito milestone Apr 7, 2022
@g-cheishvili g-cheishvili changed the title Fix/icon tabs header horizon delta theming feat(styles): icon tabs header horizon delta theming Apr 7, 2022
@netlify
Copy link

netlify bot commented Apr 7, 2022

Deploy Preview for fundamental-styles ready!

Name Link
🔨 Latest commit 108bd7f
🔍 Latest deploy log https://app.netlify.com/sites/fundamental-styles/deploys/625864edac5f5a000aa433e5
😎 Deploy Preview https://deploy-preview-3301--fundamental-styles.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@g-cheishvili g-cheishvili force-pushed the fix/icon-tabs-header-horizon-delta-theming branch from fb79c84 to db44dd0 Compare April 7, 2022 17:38
Copy link
Contributor

@platon-rov platon-rov left a comment

Choose a reason for hiding this comment

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

  • Container padding is different

Current:
image

Specs:
image

  • Label color is grey what is incorrect.

Current:
image

Specs:
image

  • Icons not centered, for not selected tab glyph color is wrong

Current:
image

Specs:
image

@g-cheishvili g-cheishvili requested a review from a team April 7, 2022 20:42
platon-rov
platon-rov previously approved these changes Apr 8, 2022
Copy link
Contributor

@platon-rov platon-rov left a comment

Choose a reason for hiding this comment

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

Padding of the header is set with sizing. By default fd-icon-tab-bar does not have any padding

I see, but accordingly to the specifications size S should be applied for small screens, M for medium and so on, and we don't apply it implicitly. It's weirdo to don't have paddings at all and i don't see anything about this variant without paddings in the wiki, maybe we should contact designers, what do you think?

For some reason Label color and font is correctly set, but visually it does not appear. Although in theming variables they exist

Inactive tab should have color: --sapTextColor, but it has color: var(--sapContent_LabelColor).

image

Copy link
Contributor

@platon-rov platon-rov left a comment

Choose a reason for hiding this comment

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

Accidentally approved.

@g-cheishvili g-cheishvili dismissed platon-rov’s stale review April 8, 2022 14:25

Accidental approval

@g-cheishvili
Copy link
Contributor Author

g-cheishvili commented Apr 8, 2022

Padding of the header is set with sizing. By default fd-icon-tab-bar does not have any padding

I see, but accordingly to the specifications size S should be applied for small screens, M for medium and so on, and we don't apply it implicitly. It's weirdo to don't have paddings at all and i don't see anything about this variant without paddings in the wiki, maybe we should contact designers, what do you think?

For some reason Label color and font is correctly set, but visually it does not appear. Although in theming variables they exist

Inactive tab should have color: --sapTextColor, but it has color: var(--sapContent_LabelColor).

image

Theme changer glitches, if you change theme to horizon, then refresh page, it will load correct variables and then you'll see that although correct vars are set, it still shows differently from wiki. Plus color is correct in that case, but font-family is incorrect

@github-actions
Copy link

This pull request is stale because it has been open 2 days with no activity. Remove stale label or comment or this will be closed in 3 days

@g-cheishvili g-cheishvili force-pushed the fix/icon-tabs-header-horizon-delta-theming branch from f349f23 to 96d5af0 Compare April 13, 2022 19:42
@mikerodonnell89 mikerodonnell89 self-requested a review April 14, 2022 17:37
@droshev droshev force-pushed the fix/icon-tabs-header-horizon-delta-theming branch from 466718d to 108bd7f Compare April 14, 2022 18:16
@droshev droshev merged commit 3dbfa87 into main Apr 14, 2022
@droshev droshev deleted the fix/icon-tabs-header-horizon-delta-theming branch April 14, 2022 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants