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.

Sign up for GitHub

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

Jump to bottom

Fixed breadcrumbs calculation and actions flow #7478

Merged
merged 7 commits into from
Dec 22, 2017
Merged

Fixed breadcrumbs calculation and actions flow #7478

merged 7 commits into from
Dec 22, 2017

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Dec 13, 2017 •
edited
Loading

@skjnldsv skjnldsv added 3. to review Waiting for reviews bug design Design, UI, UX, etc. medium regression labels Dec 13, 2017
@skjnldsv skjnldsv added this to the Nextcloud 13 milestone Dec 13, 2017
@skjnldsv skjnldsv self-assigned this Dec 13, 2017
This was referenced Dec 13, 2017
Merged
Closed
@skjnldsv skjnldsv requested a review from pixelipo December 13, 2017 10:56
@MorrisJobke
Copy link
Member

In the files app the gallery button is not pushed to the left anymore once you open the sidebar. In the past the button was moved to the left of the sidebar once it was opened. This may be crucial in the future once the sidebar is open per default for wide screens.

pixelipo
pixelipo approved these changes Dec 13, 2017
View reviewed changes
Copy link
Contributor

@pixelipo pixelipo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Exactly how it should be solved (few CSS nitpicks aside)

Some work is needed in the gallery app PR.

apps/files/css/files.scss
@@ -21,6 +21,11 @@
.actions.creatable {
position: relative;
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we don't need this anymore.

apps/files/css/files.scss Outdated
display: flex;
flex: 1 1;
.button:not(:last-child) {
margin-right: 3px;
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason for this? AFAICS, it's only applies to button.new, which doesn't need it (#uploadprogresswrapper already has a left margin).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, in case of other apps want to add their buttons, let them do it without needing to define specific margins :)

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, but then you can remove the margin from #uploadprogresswrapper

Copy link
Contributor

Choose a reason for hiding this comment

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

also, it should be #controls .button, not .actions.createable

Copy link
Member Author

Choose a reason for hiding this comment

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

All buttons should be in the .actions div now. Far better thann having buttons everywhere :)

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps, but definitely not in .createable ;)

@pixelipo pixelipo mentioned this pull request Dec 13, 2017
Merged
@pixelipo
Copy link
Contributor

Should @MorrisJobke 's issue be fixed in this PR as well? I'm made a PR to this branch with a proposed solution to it: #7484

MorrisJobke
MorrisJobke approved these changes Dec 20, 2017
View reviewed changes
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.

Together with #7484 this works fine 👍

skjnldsv and others added 6 commits December 20, 2017 13:48
@skjnldsv
Fixed breadcrumbs calculation and actions flow
7a9e65c 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@pixelipo @skjnldsv
Dynamic control bar width
228ca16 
Signed-off-by: Marin Treselj <marin@pixelipo.com>
@pixelipo @skjnldsv
Make contorls stick to the top when scrolling
eebc70c 
Signed-off-by: Marin Treselj <marin@pixelipo.com>
@MorrisJobke @skjnldsv
Properly show action icons
9e6f956 
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke @skjnldsv
Add the webkit sticky position prefix
0fef608 
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@skjnldsv
zindex fix
3105a27 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the breadcrumbs-buttons-fixes-gallery branch from 8941bf3 to 3105a27 Compare December 20, 2017 12:50
@codecov
Copy link

codecov bot commented Dec 20, 2017 •
edited
Loading

Codecov Report

Merging #7478 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #7478   +/-   ##
=========================================
  Coverage     51.17%   51.17%           
  Complexity    24886    24886           
=========================================
  Files          1602     1602           
  Lines         94750    94750           
  Branches       1368     1368           
=========================================
  Hits          48486    48486           
  Misses        46264    46264
Impacted Files Coverage Δ Complexity Δ
lib/private/Files/Cache/Propagator.php 94.93% <0%> (-1.27%) 16% <0%> (ø)
apps/files_trashbin/lib/Trashbin.php 72.53% <0%> (+0.24%) 136% <0%> (ø) ⬇️

@skjnldsv
Copy link
Member Author

Okay, fixed zindex, rebased and seems fine! :)

@skjnldsv
Fixed breadcrumbs tests
8cc90ab 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Copy link
Member Author

skjnldsv commented Dec 21, 2017 •
edited
Loading

Test fixed! There was an issue from #7051.... The test was basically useless ^^

We need to test the tests 😆

EDIT: tests passed! 🎉
PhantomJS 2.1.1 (Linux 0.0.0): Executed 1001 of 1003 (skipped 2) SUCCESS (26.416 secs / 25.362 secs)

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 21, 2017
@skjnldsv skjnldsv merged commit d0e7b0d into master Dec 22, 2017
@skjnldsv skjnldsv deleted the breadcrumbs-buttons-fixes-gallery branch December 22, 2017 08:11
@danxuliu danxuliu mentioned this pull request Dec 26, 2017
Closed
@MorrisJobke MorrisJobke mentioned this pull request Jan 2, 2018
Merged
30 tasks
This was referenced Feb 4, 2022
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pixelipo pixelipo pixelipo approved these changes

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@oparoz oparoz Awaiting requested review from oparoz

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@MariusBluem MariusBluem Awaiting requested review from MariusBluem

Assignees

@skjnldsv skjnldsv

Labels
4. to release Ready to be released and/or waiting for tests to finish bug design Design, UI, UX, etc. medium regression
Projects
None yet
Milestone
Nextcloud 13
Development

Successfully merging this pull request may close these issues.
3 participants
@skjnldsv @MorrisJobke @pixelipo