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

Export bookmarks #6652

Merged
merged 4 commits into from
Feb 6, 2017
Merged

Export bookmarks #6652

merged 4 commits into from
Feb 6, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Jan 15, 2017

  • 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).

Resolves #1002

Auditors

@bbondy @bsclifton

Test Plan

Moved to #1002 (comment)

Automated tests

  • npm run watch-test
  • npm run unittest -- --grep="Bookmarks export"

@NejcZdovc NejcZdovc force-pushed the feature/#1002-export branch 3 times, most recently from baa445d to 4a0d019 Compare January 15, 2017 19:03
@NejcZdovc NejcZdovc requested review from bbondy and bsclifton January 15, 2017 22:16
@NejcZdovc NejcZdovc self-assigned this Jan 15, 2017
@luixxiul luixxiul changed the title Added export bookmars functionality Added export bookmarks functionality Jan 16, 2017
@NejcZdovc NejcZdovc requested a review from darkdh January 16, 2017 10:24
@bsclifton bsclifton added this to the 0.13.1 milestone Jan 16, 2017
@luixxiul
Copy link
Contributor

Sweet! love to see this implemented soon :-)

@NejcZdovc NejcZdovc changed the title Added export bookmarks functionality Export bookmarks Jan 18, 2017
@bsclifton bsclifton changed the base branch from master to 0.13.1-branch January 23, 2017 23:31
@bsclifton bsclifton force-pushed the 0.13.1-branch branch 3 times, most recently from 65c7753 to 238cfd3 Compare January 25, 2017 19:19
@bsclifton bsclifton modified the milestones: 0.13.2, 0.13.1 Jan 26, 2017
@bsclifton bsclifton closed this Jan 26, 2017
@bsclifton bsclifton changed the base branch from 0.13.1-branch to master January 26, 2017 21:16
@bsclifton bsclifton reopened this Jan 26, 2017
@bbondy
Copy link
Member

bbondy commented Jan 28, 2017

@NejcZdovc would you mind rebasing this PR? Thanks!

@NejcZdovc NejcZdovc force-pushed the feature/#1002-export branch from 33d2e50 to af75a51 Compare January 28, 2017 18:38
@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Feb 2, 2017

@bradleyrichter added new icons.

image

@NejcZdovc NejcZdovc force-pushed the feature/#1002-export branch from faeae7f to 089cba0 Compare February 2, 2017 06:28
@bradleyrichter
Copy link
Contributor

@NejcZdovc Can you try adding some padding between them? About 5 px or equivalent.

And what is the best way to get our mouse-hover highlight to work? Use CSS with the SVG?

@NejcZdovc NejcZdovc force-pushed the feature/#1002-export branch from 089cba0 to 3e1c6fa Compare February 2, 2017 06:42
@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Feb 2, 2017

@bradleyrichter I was just updating padding when you wrote about it 😃 We are doing hover with CSS.

New layout (changed from 7px to 10px)
image

If I need to add some more padding please let me know

@bradleyrichter
Copy link
Contributor

ha ha...

Now I see that the new folder button looks bigger than the import/export. Can you try enlarging the import/export buttons?

@luixxiul
Copy link
Contributor

luixxiul commented Feb 2, 2017

I started feeling a bit that the import and export icons should not be on the "Folders" row (because it is not the folders that are to be imported or exported); instead right or left of the search form would be better. wdyt @bradleyrichter ?

@NejcZdovc NejcZdovc force-pushed the feature/#1002-export branch from 3e1c6fa to 90c3247 Compare February 2, 2017 06:49
@NejcZdovc
Copy link
Contributor Author

@bradleyrichter icons are aligned, maybe it's an optical thing?

image

@NejcZdovc
Copy link
Contributor Author

@luixxiul what about on the right side where you have add bookmark icon?

@bradleyrichter
Copy link
Contributor

bradleyrichter commented Feb 2, 2017

@luixxiul I think your suggestion is good. It's more hierarchically correct.

image

Add Bookmark should add a new BM into the currently selected folder so that one seems to be in the right spot.

@NejcZdovc If you can move them easily, and agree that it's clearer, go ahead. Otherwise we can finish and close this PR as is and revisit the location.

@NejcZdovc NejcZdovc force-pushed the feature/#1002-export branch from 90c3247 to 0ccd289 Compare February 2, 2017 07:18
@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Feb 2, 2017

@bradleyrichter @luixxiul I like the new position. It's implemented now. What do you guys think?

@luixxiul
Copy link
Contributor

luixxiul commented Feb 3, 2017

Looking good 👍 except when you minimize the window:

screenshot 2017-02-03 14 21 23

I'd make an adjustment in a follow up task as that's a minor issue.

@bradleyrichter
Copy link
Contributor

bradleyrichter commented Feb 3, 2017 via email

@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Feb 3, 2017

If you will prepare them before the end of 0.13.3 milestone, I can add it in this PR, otherwise I would merge this PR and add it later.

@NejcZdovc NejcZdovc force-pushed the feature/#1002-export branch from 0ccd289 to e8d7e2e Compare February 3, 2017 10:55
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

++

@bsclifton
Copy link
Member

Most tests passed with CI; those which didn't, I ran locally and verified they work. Feature is good to go! Merging

@StuartWilloughby
Copy link

This appears to have broken in 0.19.70. Clicking on Export does not generate a file.

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.

6 participants