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

Breadcrumbs refactor #7051

Merged
merged 33 commits into from
Nov 13, 2017
Merged

Breadcrumbs refactor #7051

merged 33 commits into from
Nov 13, 2017

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Nov 3, 2017

Summary here: #7051 (comment)

Fix #5431, fix #6756
Closes #5229

Rebuilding the breadcrumbs design and calculation system for a better result:

  • Flexi flexo magic
  • Calculate what elements to hide
  • Auto hide
  • Put hidden elements in a popover
  • Mobile style
  • Fix click feedback and sidebar opening
  • Animation ?
  • Fix file picker
  • Tests ⚠️
  • Performance optimisations
    • There are multiple calls for BreadCrumb.setDirectoryInfo & BreadCrumb.setDirectory which force re-renders
    • There are at least 3 BreadCrumb._resize execution on init, only one is needed

@skjnldsv skjnldsv added 2. developing Work in progress design Design, UI, UX, etc. labels Nov 3, 2017
@skjnldsv skjnldsv added this to the Nextcloud 13 milestone Nov 3, 2017
@skjnldsv skjnldsv self-assigned this Nov 3, 2017
@codecov
Copy link

codecov bot commented Nov 4, 2017

Codecov Report

Merging #7051 into master will increase coverage by 0.01%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #7051      +/-   ##
============================================
+ Coverage     50.72%   50.74%   +0.01%     
  Complexity    24478    24478              
============================================
  Files          1581     1581              
  Lines         93578    93560      -18     
  Branches       1359     1353       -6     
============================================
+ Hits          47471    47475       +4     
+ Misses        46107    46085      -22
Impacted Files Coverage Δ Complexity Δ
core/js/js.js 63.55% <ø> (+1.72%) 0 <0> (ø) ⬇️
core/js/oc-dialogs.js 0.43% <0%> (ø) 0 <0> (ø) ⬇️
apps/files_trashbin/lib/Expiration.php 90.32% <0%> (-1.62%) 29% <0%> (ø)
lib/private/Server.php 83.29% <0%> (-0.13%) 125% <0%> (ø)
apps/files_trashbin/lib/Trashbin.php 72.53% <0%> (+0.24%) 136% <0%> (ø) ⬇️
lib/private/Security/CertificateManager.php 92.07% <0%> (+0.99%) 39% <0%> (ø) ⬇️

@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 4, 2017

@jancborchardt @juliushaertl @nextcloud/javascript
Do we have a specific function that handle a menu open/close and a click outside menu behaviour?

@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 7, 2017

BreadCrumb.setDirectoryInfo is used to re-render the apps that add their buttons in the breadcrumb (like the sharing button) so we can't disabled the second rendering after BreadCrumb.setDirectory.

Pretty much the same for the resize function! 😕

Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Nov 12, 2017
@skjnldsv
Copy link
Member Author

@rullzer, @MorrisJobke, @juliushaertl, @jancborchardt, @ChristophWurst, please help and test it. 😭

@MorrisJobke
Copy link
Member

Even with the latest changes the popup does not show up in Chrome nor in Safari and there is nothing in the console :/

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works now (after disabling the gallery app) 👍

.drone.yml Outdated
- JAVA_OPTS=-Dselenium.LOGGER.level=WARNING
when:
matrix:
TESTS: acceptance
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course, it was for testing purposes only! :)

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

With the Gallery fix nextcloud/gallery#325 it seems good now! :) Great work!

@MorrisJobke
Copy link
Member

@skjnldsv Is this ready for merge or not? (because the label says something different ;))

@skjnldsv
Copy link
Member Author

It is, I was waiting for other tests to be sure if this was a pr issue or something from @blizzz dev environnement. :)

@skjnldsv skjnldsv added 3. to review Waiting for reviews design Design, UI, UX, etc. enhancement feature: files medium and removed 2. developing Work in progress design Design, UI, UX, etc. enhancement feature: files medium labels Nov 13, 2017
@MorrisJobke MorrisJobke merged commit ff2d443 into master Nov 13, 2017
@MorrisJobke MorrisJobke deleted the breadcrumbs-refactor branch November 13, 2017 11:19
@blizzz
Copy link
Member

blizzz commented Nov 13, 2017

Still broken from me, also pristine from git -.- Ill open issues

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

Successfully merging this pull request may close these issues.

Share icon in breadcrumbs is not working Breadcrumb glitch in trashbin for sub folders
5 participants