Skip to content
This repository has been archived by the owner on May 13, 2024. It is now read-only.

Shields v2.1 #382

Merged
merged 28 commits into from
Feb 28, 2019
Merged

Shields v2.1 #382

merged 28 commits into from
Feb 28, 2019

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Feb 20, 2019 •
edited by petemill
Loading

shield

address brave/brave-browser#2565
address brave/brave-browser#1894
address brave/brave-browser#1892

Changes

Mostly refactor everything under shields/

Test plan

Note: There was a change in the Toggle element that is also shared with the private new tab. I'll handle this when porting to brave-core.

Note 2: Design feedback and review was made by @karenkliu and should be good to go unless her or someone else from the design team says otherwise.

  1. Ensure you can navigate through shields using the tab key and that the space key trigger the action that onClick would trigger.
Link / storybook path to visual changes

Integration

  • Does this contain changes to src/components or src/

    • Will you publish to npm immediately after this PR, or wait until sometime in the future?
    • Incompatible API change to something existing (major version increase)
    • Adding new backwards-compatible functionality? (minor version increase)
    • Fixing a bug backwards-compatibly? (patch version increase)

  • Does this contain changes to src/features for brave-core?

    • Are there non backwards-compatible changes required for brave-core? Do not merge until brave-core PR is approvable. Link to brave-core PR:
    • Will you create brave-core PR to update to this commit after it is merged?
    • Wants uplift to brave-core feature branch?
      • When uplift-approved, merge to brave-core-0.VV.x feature branch
      • Create additional brave-core PRs for each feature branch to update commit

@cezaraugusto cezaraugusto self-assigned this Feb 20, 2019
rossmoody
rossmoody reviewed Feb 20, 2019
View reviewed changes
src/features/shields/structure.ts
box-sizing: border-box;
background-image: url(${Background});
background-repeat: no-repeat;
background-position: ${p => p.status === 'enabled' ? '0 -15px' : '0 0'};
Copy link
Contributor

Choose a reason for hiding this comment

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

100% personal preference, but thought this element was riding a little high:
background-position: ${p => p.status === 'enabled' ? '0 -5px' : '0 0'};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good but deferring to @karenkliu

@rossmoody
Copy link
Contributor

Love the interactivity polish here. So nice. 🎉 👍

karenkliu
karenkliu previously approved these changes Feb 20, 2019
View reviewed changes
Copy link

@karenkliu karenkliu left a comment

Choose a reason for hiding this comment

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

Thanks @rossmoody for your comments - I was initially confused as well, but Cezar has implemented a Tab + Space rather than Tab + Enter to trigger the interaction and it works as expected.

@cezaraugusto I have tested your focus states in the link you sent https://brave-ui-959w0ii2u.now.sh and it looks good to go.

@rossmoody
Copy link
Contributor

Duh, I was pressing enter and not spacebar. That's a great addition. No issue.

Just to be sure. This is what I'm seeing: https://d.pr/i/9kiate which seems tight in comparison to your Sketch doc: https://d.pr/i/SU6cWx Fixed via: #382 (comment)

@tildelowengrimm
Copy link

Will the production version support enter rather than space, or is it all space4lyf?

@karenkliu karenkliu dismissed their stale review February 20, 2019 20:03

Dismissing my review - I didn't realize this PR would close #2565 and #1892 as well. There are still pending changes for those issues.

@cezaraugusto
Copy link
Contributor Author

@tomlowenthal disabled users should do their flow with the keys they expect to work which are tab for navigation, space for toggles and space and Enter for button actions. not usual to have Enter in other actions, although it can be implemented without any burden

@rossmoody rossmoody self-requested a review February 22, 2019 20:38
@tildelowengrimm
Copy link

Thanks Cezar. I guess I'm not up on the keyboard-a11y norms. Definitely we should do whatever is the standard/expected thing. And thanks for showing me <kbd> markup. I didn't know about that before, and it's a neat technique.

@karenkliu
Copy link

I dismissed my review but after going over these issues with Cezar, this PR addresses #2566, #1894, and #1892 - good to go. 👍

rossmoody
rossmoody approved these changes Feb 27, 2019
View reviewed changes
Copy link
Contributor

@rossmoody rossmoody left a comment

Choose a reason for hiding this comment

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

👍

petemill
petemill previously requested changes Feb 28, 2019
View reviewed changes
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

Great! Just some feedback on the icons where I think the generated file is not working out.

src/features/shields/media/arrowDown.tsx Outdated Show resolved Hide resolved
@cezaraugusto
Copy link
Contributor Author

after deliberating with @rossmoody and @petemill we're going to use the default brave-ui carat icon for now, which will be changed to something more closer to the original Shield spec defined TBD in a follow-up. this is what is looks like right now:

shot

since there are no other code comments I'm going to land this so we can quickly start porting everything to brave-core

@cezaraugusto cezaraugusto merged commit b7d1932 into master Feb 28, 2019
@cezaraugusto cezaraugusto deleted the shields-new branch February 28, 2019 22:42
cezaraugusto added a commit that referenced this pull request Mar 1, 2019
@cezaraugusto
Revert "Merge pull request #382 from brave/shields-new"
4f055f3 
This reverts commit b7d1932, reversing
changes made to 00d51ca.
NejcZdovc added a commit that referenced this pull request Mar 7, 2019
@NejcZdovc
Revert "Merge pull request #382 from brave/shields-new"
b4ecedf 
This reverts commit b7d1932, reversing
changes made to 00d51ca.
cezaraugusto added a commit that referenced this pull request Mar 7, 2019
@cezaraugusto
Revert "Merge pull request #382 from brave/shields-new"
77240a9 
This reverts commit b7d1932, reversing
changes made to 00d51ca.
cezaraugusto added a commit that referenced this pull request Mar 8, 2019
@cezaraugusto
Revert "Revert "Merge pull request #382 from brave/shields-new""
83f5322 
This reverts commit 77240a9.
cezaraugusto added a commit that referenced this pull request Mar 13, 2019
@cezaraugusto
Revert "Revert "Merge pull request #382 from brave/shields-new""
c805978 
This reverts commit 77240a9.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rossmoody rossmoody rossmoody approved these changes

@karenkliu karenkliu karenkliu left review comments

@petemill petemill petemill left review comments

@bsclifton bsclifton Awaiting requested review from bsclifton

@tildelowengrimm tildelowengrimm Awaiting requested review from tildelowengrimm

Assignees

@cezaraugusto cezaraugusto

Labels
features/shields
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cezaraugusto @rossmoody @tildelowengrimm @karenkliu @petemill