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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(accordion): update focus highlight #1401

Merged
merged 3 commits into from
Jun 19, 2019

Conversation

qqingvarqq
Copy link
Contributor

Fixes #1323

Description

Removed position relative because it changes stacking, which causes to hiding focus highlight at the bottom, as described in the linked issue.

This fix also fixes inconsistent with 'outlining content', when we add overrides, because outline: 2px solid #F6BA8B; is outside box model that's mean that we should show full outlining or hide them completely.
(It's from the documentation site https://baseweb.design/components/accordion/#overrides)
Before:
before

After:
after

If you need to hide all content even outside the box-model I can make appropriate changes for that.

Scope

  • Patch: Bug Fix

@CLAassistant
Copy link

CLAassistant commented Jun 14, 2019

CLA assistant check
All committers have signed the CLA.

@gergelyke
Copy link
Contributor

it seems like it introduces a visual regression in one of the table examples

Screen Shot 2019-06-14 at 1 58 06 PM

can you please take a look at that?

@qqingvarqq
Copy link
Contributor Author

Sure
The plus icon moved to the left because previously it was hard coded by position absolute and left: 12px, and now there is no position relative in the parent.
I changed CSS in this table so the plus icon is no longer hardcoded.
I'm new in this project so if something wrong, I can make appropriate changes to this PR

@qqingvarqq
Copy link
Contributor Author

qqingvarqq commented Jun 17, 2019

@gergelyke Thank you for your time. I see that the screener check was failed again. Can you provide from where regression coming, because I check storybook and it seems that stories don't have regression. Maybe I miss something?

@chasestarr
Copy link
Collaborator

it's specifically when running IE 11

@qqingvarqq
Copy link
Contributor Author

I looked at these changes when running IE 11. (table-cells page)
Master Branch (IE):
master
My branch (fix-accordion-ui) (IE):
fix-ui

Actually, those changes are good, because

  1. fixed vertical centering of plus icon under ie,
  2. The 'Expandable' column is not overlapping 'Amount' column, under ie
    ==============
    Last one change
  3. 'Expanded' column has a normal width under ie
    So waiting for feedback

@gergelyke
Copy link
Contributor

I looked at these changes when running IE 11. (table-cells page)
Master Branch (IE):
master
My branch (fix-accordion-ui) (IE):
fix-ui

Actually, those changes are good, because

  1. fixed vertical centering of plus icon under ie,
  2. The 'Expandable' column is not overlapping 'Amount' column, under ie

    Last one change
  3. 'Expanded' column has a normal width under ie
    So waiting for feedback

oh, that makes perfect sense, thanks for the fix! I've just started running the CI on it 🤞

@qqingvarqq
Copy link
Contributor Author

It's very strange that build was failed.
There are no changes to Slack link, and Slack link is working.
├─BROKEN─ https://join.slack.com/t/baseui/shared_invite/enQtNDI0NTgwMjU0NDUyLTk3YzM1NWY2MjY3NTVjNjk3NzY1MTE5OTI4Y2Q2ZmVkMTUyNDc1MTcwYjZhYjlhOWQ2M2NjOWJkZmQyNjFlYTA (ERRNO_ECONNREFUSED)

@gergelyke gergelyke merged commit 03e3fb4 into uber:master Jun 19, 2019
@gergelyke
Copy link
Contributor

thanks for the contribution @qqingvarqq 👏

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.

Accordion UI Bug
6 participants