Fixed gallery button insertion script and design fix #336

skjnldsv · 2017-12-13T10:54:47Z

Fixes nextcloud/server#7374
Needs nextcloud/server#7478

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

MorrisJobke · 2017-12-13T11:17:04Z

The gallery controls look still a bit weird:

skjnldsv · 2017-12-13T11:19:24Z

Ouuuups, let me fix it

oparoz · 2017-12-13T11:33:12Z

When testing, it's important to also try with public shares as the layout is completely different.

pixelipo

I think this needs more work - I'll add notes in a separate comment

pixelipo · 2017-12-13T12:27:36Z

3 issues visible here:

folder breadcrumb is not in line with the rest
some icons not in line with the rest
icons in right container are not pushed to the right edge (flex is needed here as well plus something to account for no sidebar...)
It's even worse (but not visible here) once the loader is on:

In general, button in that controls bar seem to each have their own CSS ruleset. It would be nice to get them to work with as little custom css as possible

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

skjnldsv · 2017-12-13T12:43:54Z

Fixed a lot, please re-test @pixelipo :)

pixelipo

Looks better for logged in user, but:

breadscrumbs folders are still not vertically aligned
~~buttons are 40x36px (shouldn't they be squared?)~~
~~#controls is 44px high, so margin-top for buttons should be 4px (or 2px if we go for 40x40 square buttons)~~
public view looks like crap:

pixelipo · 2017-12-13T12:58:51Z

css/styles.css

@@ -29,8 +29,12 @@ div.crumb.last a {
 	width: 40px;
 }

-#controls > .right {
-	float: right;
+#controls .button:not(:last-child) {


this should go to nextcloud/server#7478

Gallery uses a lot of similar styles but does not includes any of them (yes, we need to rebuild the entire app), therefor, there are a lot of multiples files/properties used.

skjnldsv · 2017-12-13T13:10:09Z

#controls is 44px high, so margin-top for buttons should be 4px (or 2px if we go for 40x40 square buttons)

Buttons are always 44x44 on nextcloud! :)
I'll fix it asap

pixelipo · 2017-12-13T13:16:19Z

@skjnldsv be careful with 44x44 - it means we also need to make #controls {height: 50px} and #controls button {margin-top: 3px) - so that's a commit for nextcloud/server#7374

skjnldsv · 2017-12-13T13:19:05Z

Well, I guess it won't be 44x44 then! I don't want to try and break everything! :O
😭

pixelipo · 2017-12-13T13:27:06Z

@skjnldsv ok, I've created a new issue for that: nextcloud/server#7481

skjnldsv · 2017-12-13T13:31:38Z

breadscrumbs folders are still not vertically aligned

What do you mean by that?

pixelipo · 2017-12-13T14:29:16Z

@skjnldsv look here - folder breadcrumb is 2px lower than the rest:

skjnldsv · 2017-12-13T14:31:54Z

@pixelipo And it's the opposite on the files app, the buttons are 2px lower...

pixelipo · 2017-12-13T14:38:09Z

@skjnldsv that problem would be solved with proper button size and margins

oparoz · 2017-12-13T16:02:32Z

@pixelipo the gallery app doesn't even use the new breadcrumbs system

Very true, by design apps should never rely on components delivered in the server, which is annoying.

MorrisJobke · 2017-12-20T11:25:29Z

I pushed a fix to this branch to properly work with nextcloud/server#7484 this is now good to be merged. 👍

MorrisJobke · 2017-12-20T11:25:46Z

breadscrumbs folders are still not vertically aligned

Let's skip this for this PR.

Signed-off-by: Morris Jobke <hey@morrisjobke.de>

MorrisJobke · 2017-12-20T11:36:21Z

I didn't had the second commit from @skjnldsv (27f4c4e) - with this the file switcher is now misplaced. Without that commit it is properly shown.

pixelipo · 2017-12-20T12:54:52Z

@MorrisJobke I'll make another PR in a second

Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>

skjnldsv · 2017-12-20T14:00:47Z

css/styles.css

@@ -20,16 +17,19 @@ body:after {
 	transition: transform .1s ease-in;
 }

-div.crumb.last a {
-	cursor: default;
+/* Two ugly hacks to fix breadcrumbs in Gallery */


Shouldn't this go in the gallery then? :)

EDIT: wrong pr, we are in the gallery 🙈 😆

Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>

pixelipo · 2017-12-20T14:19:37Z

That was last commit from me - it fixes public view. Before:

After:

Please review

skjnldsv · 2017-12-20T14:30:45Z

@pixelipo you're awesome! Perfect here!

Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>

pixelipo · 2017-12-20T14:37:20Z

Sorry, I lied - I found out that my "ugly hack" was causing a bug when only "Home" is the only breadcrumb. I've fixed it now, with the downside (which cannot be fixed without total rewrite of Gallery breadcrumbs) is that in case ellipsis is the last breadcrumb, it doesn't have the expected margin-right.

Good:

Still good:

Bad:

I guess we can live with that for now ;)

Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>

pixelipo · 2017-12-20T14:47:24Z

My last (no, really) commit fixes the hover inconsistency issue mentioned by @MariusBluem in the nextcloud/server#7374

skjnldsv · 2017-12-21T15:38:48Z

Another bump here?

pixelipo · 2017-12-22T12:04:21Z

@skjnldsv travis-ci is failing:

1) ConnectWithTokenCest: Make sure i can get the thumbails
 Test  tests/api/ConnectWithTokenCest.php:connectToThumbnails
 Step  See response contains ""fileid":"274","status":200"
 Fail  REST response contains
Failed asserting that 'event: preview
data: {"preview":null,"mimetype":"application\/postscript","fileid":"274","status":500}

oparoz · 2017-12-22T12:40:17Z

@skjnldsv travis-ci is failing:

Unrelated. I still haven't found the time to trace the regression. Could have been broken by a change in the server. There is an issue opened about it.

skjnldsv · 2018-01-03T12:32:18Z

@MorrisJobke @rullzer A review here? 🙏

MorrisJobke

Tested and works 👍

MorrisJobke · 2018-01-03T15:26:59Z

@skjnldsv @pixelipo @oparoz Does the gallery app now only support 13+ or is this still working with 12 and before? I'm asking because otherwise the info.xml needs to be updated to not publish not-working apps in the appstore.

skjnldsv · 2018-01-03T15:34:05Z

13+ I think. We need to test on stable12.

danxuliu · 2018-01-04T15:35:18Z

Does the gallery app now only support 13+ or is this still working with 12 and before?

Even if this pull request worked on 12 (and I made a quick check and there seems to be some issues) due to #331 the slideshow icons are not visible on a black background on Nextcloud 12:

rullzer · 2018-01-04T15:42:14Z

Ok lets bump to 13 only then we have the stable branches anyways

Fixed gallery button insertion script and design fix

1089b8c

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

skjnldsv added 3. to review Waiting for reviews bug Something isn't working design Related to the design regression Regression of a previous working feature labels Dec 13, 2017

skjnldsv added this to the Nextcloud 13 milestone Dec 13, 2017

skjnldsv self-assigned this Dec 13, 2017

skjnldsv requested review from MorrisJobke, oparoz, jancborchardt and MariusBluem December 13, 2017 10:54

This was referenced Dec 13, 2017

Fixed breadcrumbs calculation and actions flow nextcloud/server#7478

Merged

Position of Gallery-button and inconsistent behavior on hover nextcloud/server#7374

Closed

skjnldsv requested a review from pixelipo December 13, 2017 10:56

pixelipo suggested changes Dec 13, 2017

View reviewed changes

Fixed multiplde design flaws

27f4c4e

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

pixelipo suggested changes Dec 13, 2017

View reviewed changes

Fix CSS to work around wrong gallery DOM structure

a418fad

Signed-off-by: Morris Jobke <hey@morrisjobke.de>

Fix button and breadcrumb margins/paddings

55dd750

Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>

skjnldsv commented Dec 20, 2017

View reviewed changes

Fix public sharing view

c44d7f0

Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>

pixelipo approved these changes Dec 20, 2017

View reviewed changes

Fix a bug in the "ugly hack" when Home is the last breadcrumb

a82ba8f

Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>

Fix inconsistent hover behaviour for the gallery button

301d8cf

Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>

MorrisJobke approved these changes Jan 3, 2018

View reviewed changes

MorrisJobke merged commit 3639b62 into master Jan 3, 2018

MorrisJobke deleted the gallery-button-insertion-fix branch January 3, 2018 15:26

danxuliu mentioned this pull request Jan 4, 2018

This version in NC13 only #347

Merged

danxuliu mentioned this pull request Jan 17, 2018

13.0.0 RC1 Gallery while logged contains fewer photos than the same gallery using the share link #364

Closed

danxuliu mentioned this pull request Feb 11, 2018

New rows are not added when a very short window is resized vertically #136

Closed

Fixed gallery button insertion script and design fix #336

Fixed gallery button insertion script and design fix #336

Conversation

skjnldsv commented Dec 13, 2017 • edited Loading

MorrisJobke commented Dec 13, 2017

skjnldsv commented Dec 13, 2017

oparoz commented Dec 13, 2017

pixelipo left a comment

Choose a reason for hiding this comment

pixelipo commented Dec 13, 2017 • edited Loading

skjnldsv commented Dec 13, 2017

pixelipo left a comment • edited Loading

Choose a reason for hiding this comment

pixelipo Dec 13, 2017

Choose a reason for hiding this comment

skjnldsv Dec 13, 2017

Choose a reason for hiding this comment

pixelipo Dec 13, 2017

Choose a reason for hiding this comment

skjnldsv commented Dec 13, 2017

pixelipo commented Dec 13, 2017

skjnldsv commented Dec 13, 2017

pixelipo commented Dec 13, 2017

skjnldsv commented Dec 13, 2017

pixelipo commented Dec 13, 2017

skjnldsv commented Dec 13, 2017

pixelipo commented Dec 13, 2017

oparoz commented Dec 13, 2017

MorrisJobke commented Dec 20, 2017

MorrisJobke commented Dec 20, 2017

MorrisJobke commented Dec 20, 2017

pixelipo commented Dec 20, 2017

skjnldsv Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

pixelipo commented Dec 20, 2017 • edited Loading

skjnldsv commented Dec 20, 2017

pixelipo commented Dec 20, 2017

pixelipo commented Dec 20, 2017 • edited Loading

skjnldsv commented Dec 21, 2017

pixelipo commented Dec 22, 2017

oparoz commented Dec 22, 2017

skjnldsv commented Jan 3, 2018

MorrisJobke left a comment

Choose a reason for hiding this comment

MorrisJobke commented Jan 3, 2018

skjnldsv commented Jan 3, 2018

danxuliu commented Jan 4, 2018

rullzer commented Jan 4, 2018

skjnldsv commented Dec 13, 2017 •

edited

Loading

pixelipo commented Dec 13, 2017 •

edited

Loading

pixelipo left a comment •

edited

Loading

skjnldsv Dec 20, 2017 •

edited

Loading

pixelipo commented Dec 20, 2017 •

edited

Loading

pixelipo commented Dec 20, 2017 •

edited

Loading