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(button): defensively set hover/focus/active styles from legacy globals #205

Merged
merged 2 commits into from Nov 27, 2019

Conversation

@SiTaggart
Copy link
Collaborator

SiTaggart commented Nov 27, 2019

We need to set some properties on buttons defensively when faced with legacy global styles in the likes of Console, when setting a button to be as an anchor.

a { ... }
a:hover { ... }

These override our styles as we rely on them not being set. Color and Text decoration seem to be the main ones I've spotted so far in console. There maybe more but we can add to the list one we find them.

Setting these explicitly should fix this for color and text-decoration

@now

This comment has been minimized.

Copy link

now bot commented Nov 27, 2019

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

🔍 Inspect: https://zeit.co/twilio-dsys/paste/7vc5z25v1
🌍 Preview: https://paste-git-fix-buttonas-link-global-defence.twilio-dsys.now.sh

@SiTaggart SiTaggart requested a review from aayushpi Nov 27, 2019
@aayushpi

This comment has been minimized.

Copy link
Contributor

aayushpi commented Nov 27, 2019

How scalable is this approach?

@SiTaggart

This comment has been minimized.

Copy link
Collaborator Author

SiTaggart commented Nov 27, 2019

How scalable is this approach?

For now this is fine. The approach I'm taking is that I'm only resetting the things we absolutely can't mitigate by the consumer rewriting their components. I don't think we should optimize for a world where you drop in a component into some bootstrap/legacy code soup and expect it to work, that definitely won't scale.

So for example, to get the buttons looking ok in console nav, I just removed a little unnecessary bootstrap that was surrounding it. That fixed most of it. We can't defend against legacy component / library styles, so the expectation should be set that you need to remove stuff that is breaking our component. But once that stuffs gone, I'm left with the global element styles that no one can get around. So we should defend against those.

There will be an upper limit where we won't be able to do much against legacy styling and make it a feasible product to maintain, so the burden does lay on the app owner to tidy up to allow seemless consumption of a new language.

@aayushpi

This comment has been minimized.

Copy link
Contributor

aayushpi commented Nov 27, 2019

Are there scenarios where we’d have to remove these styles? Would these encourage people to continue applying global properties?

@SiTaggart

This comment has been minimized.

Copy link
Collaborator Author

SiTaggart commented Nov 27, 2019

Are there scenarios where we’d have to remove these styles?

Nah, not outside of use changing the design of the component

Would these encourage people to continue applying global properties?

I doubt it, and if it does you've placed yourself firmly into "bad actor" category anyway

@SiTaggart SiTaggart merged commit 2a9ce57 into master Nov 27, 2019
8 checks passed
8 checks passed
Semantic Pull Request ready to be squashed
Details
ci/circleci: applitools Your tests passed on CircleCI!
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: prettier Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
now Deployment has completed
Details
scm/applitools No baseline conflicts found! (0 changes found)
Details
tests/applitools All visual tests passed! (29 tests)
Details
@SiTaggart SiTaggart deleted the fix/button/as-link-global-defence branch Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.