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

Bookmarks now show under Bookmarks menu #3055

Merged
merged 4 commits into from
Aug 13, 2016
Merged

Bookmarks now show under Bookmarks menu #3055

merged 4 commits into from
Aug 13, 2016

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Aug 9, 2016

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Ran git rebase -i to squash commits if needed.
  • Adds user's bookmarks under Bookmarks menu [menu temporarily disabled, code still there].
    • Manually applied patch from issue
    • Small refactor in siteUtil.js; trying to make sure bookmark code is consistent between bookmarks menu and bookmarks toolbar.
    • Added subfolders to bookmarks in menu
    • Folder menu items with 0 children no longer include empty submenu
    • Refactored code in menu.js; all submenus are defined above the template definition.
    • Bookmark menu selections now respect secondary keys
  • Reworked menu so that it only ever builds the static items once. Fixes Menu should only be built once #3022
    • The Bookmarks/History menus will now properly rebuild themselves when the appStore's sites list changes (site is bookmarked, new site added to history)
    • Fixed bug where initial browser's initial state thinks the first page loaded is NOT bookmarked, even when it is. Fixes Bookmark status not considered on first page browser loads #3098
  • Testing updates to siteUtils
    • Lots of unit tests
    • Updated siteUtil.getBookmarks to default to [] if no items found
    • Adds npm tasks for running unit tests OR spectron tests in isolation (not the whole suite)

@bsclifton
Copy link
Member Author

Ready for review 😄

cc: @bridiver @bbondy

@bbondy
Copy link
Member

bbondy commented Aug 9, 2016

I'd like to wait until #3022 is fixed as well. Could you do another commit for that in this same PR?

@bsclifton
Copy link
Member Author

Sure- I'll start looking at this tomorrow

@bsclifton
Copy link
Member Author

bsclifton commented Aug 10, 2016

Ready for review! 😄 Please note that this does NOT enable favicons in the menu, because those are more complicated. I've captured that information in #3050

cc: @bbondy @bridiver

- Small refactor in siteUtil.js; trying to make sure bookmark code is consistent, because bookmarks menu will use similar code to the bookmarks toolbar.
- Subfolders now shown in menu
- Folder menu items with 0 children no longer include empty submenu
- Refactored code in menu.js; all submenus are defined above the template definition.
- Bookmark menu selections now respect secondary keys
- Fixes #1993
- Created siteUtilTest file, moved some existing tests over, and added more tests
…the unit tests. uitest will ONLY run the spectron tests. You can continue to use the grep syntax to run specific tests.
 #3022

- The Bookmarks/History menus will now properly rebuild themselves when the appStore's sites list changes (site is bookmarked, new site added to history)
- Fixed bug where initial browser's initial state thinks the first page loaded is NOT bookmarked, even when it is. Fixes #3098
- Updated siteUtil.getBookmarks to default to [] if no items found
…next step would be finding an

efficient way to only modify the history and bookmarks menus.
@bsclifton
Copy link
Member Author

bsclifton commented Aug 13, 2016

@bbondy per your comment, I backed out the change that adds the dynamic parts of the bookmarks menu.

It should be safe to merge this now as it won't rebuild the menu. I'm working on the history menu right now too (separate of this), so I'll be sure to dig in and properly handle dynamic menus 😄

@bbondy bbondy added this to the 0.11.6dev milestone Aug 13, 2016
@bbondy
Copy link
Member

bbondy commented Aug 13, 2016

great, thanks! Merging. This won't be in 0.11.5 btw which is good anyway because it'll give us time to find any issues with the menus.

@alexwykoff
Copy link
Contributor

For QA: To be clear, this is not inclusive of the hamburger menu.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants