Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Refactor context menu #11528

Merged
merged 2 commits into from
Nov 21, 2017
Merged

Refactor context menu #11528

merged 2 commits into from
Nov 21, 2017

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Oct 14, 2017 •
edited
Loading

Closes #11354

Auditors: @cezaraugusto

Test Plan:

  1. npm run test -- --grep='ContextMenu'
  2. npm run test -- --grep='bookmarksToolbar'
  3. npm run test -- --grep='tab tests'

For QA team:

  • Make sure context menu items in the URL bar work
  • Make sure context menu items on content work with no selected text.
  • Make sure context menu items on content work with selected text.
  • Make sure context menu items on content work inside an editable control on about:styles (input, textarea, or contenteditable).

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@luixxiul luixxiul self-assigned this Oct 14, 2017
@codecov-io
Copy link

codecov-io commented Oct 14, 2017 •
edited
Loading

Codecov Report

Merging #11528 into master will increase coverage by 0.02%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master   #11528      +/-   ##
==========================================
+ Coverage   53.38%    53.4%   +0.02%     
==========================================
  Files         274      274              
  Lines       26020    26034      +14     
  Branches     4170     4172       +2     
==========================================
+ Hits        13891    13904      +13     
- Misses      12129    12130       +1
Flag Coverage Δ
#unittest 53.4% <90.47%> (+0.02%) ⬆️
Impacted Files Coverage Δ
app/renderer/components/styles/theme.js 100% <ø> (ø) ⬆️
app/renderer/components/styles/global.js 100% <ø> (ø) ⬆️
...r/components/common/contextMenu/contextMenuItem.js 53.79% <90.47%> (+3.79%) ⬆️

NejcZdovc
NejcZdovc reviewed Oct 14, 2017
View reviewed changes
test/bookmark-components/bookmarksToolbarTest.js Outdated
@@ -55,7 +55,7 @@ describe('bookmarksToolbar', function () {
.waitForVisible('[data-test-id="bookmarkToolbarButton"][title=demo1]')
.click(bookmarksToolbar)
.click('[data-test-id="bookmarkToolbarButton"][title=demo1]')
.waitForVisible('.contextMenuItemText[data-l10n-id=emptyFolderItem]')
.waitForVisible('[data-test-id="contextMenuItemText"][data-l10n-id=emptyFolderItem]')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we maybe use variables here as well?

Like: .waitForVisible(``${contextMenuItemText}[data-l10n-id=emptyFolderItem]``)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Locally I've replaced al CSS and l10n selectors (#8735) and that will be addressed with / after it

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure that I follow. What I was suggesting is that we use variable, so that if data-test-id is changed we can only change variable and everything will work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what I'm saying is that we will replace the variables all together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened an issue for that task: #11574

Copy link
Contributor

Choose a reason for hiding this comment

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

ok sounds good

@luixxiul luixxiul added this to the 0.22.x (Nightly Channel) milestone Oct 16, 2017
@luixxiul luixxiul mentioned this pull request Oct 17, 2017
Closed
@bbondy bbondy modified the milestones: 0.22.x (Nightly Channel), Backlog Oct 25, 2017
@luixxiul luixxiul removed the request for review from petemill November 14, 2017 17:19
Suguru Hirahara added 2 commits November 19, 2017 17:19
Convert contextMenu
cd6bc7d 
Closes #11354

- Remove contextMenu.less

TODO: change accelerator to #fff on hover

Auditors:

Test Plan:
1. npm run test -- --grep='ContextMenu'
2. npm run test -- --grep='bookmarksToolbar'
3. npm run test -- --grep='tab tests'
Export color properties to theme.js
eafcc89 
Adresses #11354

Auditors:

Test Plan:
cezaraugusto
cezaraugusto approved these changes Nov 21, 2017
View reviewed changes
Copy link
Contributor

@cezaraugusto cezaraugusto left a comment

Choose a reason for hiding this comment

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

++++++++++++++

@cezaraugusto cezaraugusto merged commit 8675459 into brave:master Nov 21, 2017
@luixxiul luixxiul deleted the refactor-contextMenu branch November 21, 2017 06:26
@luixxiul luixxiul mentioned this pull request Nov 21, 2017
Closed
@luixxiul luixxiul modified the milestones: Triage Backlog, 0.22.x (Nightly Channel) Nov 21, 2017
@bbondy bbondy modified the milestones: 0.22.x (Developer Channel), 0.23.x (Nightly Channel) Feb 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@NejcZdovc NejcZdovc NejcZdovc left review comments

@cezaraugusto cezaraugusto cezaraugusto approved these changes

Assignees

@luixxiul luixxiul

Labels
feature/context-menu QA/test-plan-specified refactoring/aphrodite
Projects
None yet
Milestone
0.23.x (Release Channel)
Development

Successfully merging this pull request may close these issues.
5 participants
@luixxiul @codecov-io @cezaraugusto @NejcZdovc @bbondy