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

Adds flickr style gradient to the gallery images. #456

Closed
wants to merge 1 commit into from

Conversation

raghunayyar
Copy link
Member

As we discussed at the conference (#286), we wanted to use gradients which are something on the lines of Flickr. I have added the exact same gradient here as of now. @jancborchardt would you like to tweak it a bit so that it satisfies all our corner cases?

@jancborchardt
Copy link
Member

What do you think @oparoz @owncloud/designers?

@raghunayyar can you post a screenshot so more people can review directly?

@oparoz
Copy link
Contributor

oparoz commented Oct 27, 2015

I haven't looked in details yet. I'm a bit worried about the new width, height and positioning of the element, because it may interfere with future work, so it will take a bit of time to get that checked.

@raghunayyar
Copy link
Member Author

Sure : natural state looks like this
screen shot 2015-10-28 at 12 13 16 pm

hovered state looks like this
screen shot 2015-10-28 at 12 13 27 pm

@jancborchardt
Copy link
Member

@raghunayyar nice! How's it on a brighter image?

position: absolute;
width: 100%;
height: 100%;
transition: opacity 200ms linear;
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

I have to figure out things with Atom. lol, will push the fix asap.

@raghunayyar raghunayyar force-pushed the flickr-style-gradients branch from ae96f60 to be343a1 Compare October 29, 2015 16:28
@raghunayyar
Copy link
Member Author

@jancborchardt usual state
screen shot 2015-10-29 at 9 57 29 pm

hovered state
screen shot 2015-10-29 at 9 57 34 pm

@oparoz
Copy link
Contributor

oparoz commented Oct 29, 2015

@raghunayyar Would it be possible to see the same images with the current style (pre-PR)? I know it's a bit of work, but it would be great to be able to quickly see the difference when looking at the before and after picture for both the standard and hovered state.

@jancborchardt
Copy link
Member

Looks awesome! :)

@jancborchardt
Copy link
Member

Just tested this and it looks quite awesome, way better than the current approach. However, 3 grave problems:

  • albums can not be opened, and album titles float on the top. Instead they should be shown on the bottom and have the same fade as for images, but displayed permanently.
  • images can not be opened
  • the ellipsis of long file names is broken. Should be forced to one line.

@oparoz
Copy link
Contributor

oparoz commented Nov 2, 2015

Thanks for testing. I was afraid the div changes would break stuff... Hopefully there is a way around that.

@jancborchardt
Copy link
Member

I guess just changing the opening handler would fix it. But I’m not JS-savvy enough to be sure. ;)

@oparoz oparoz added this to the 9.0-current milestone Nov 3, 2015
@raghunayyar
Copy link
Member Author

@jancborchardt will look into it asap.

@oparoz
Copy link
Contributor

oparoz commented Nov 3, 2015

There is also the problem of white letters on white background.

whiteonwite

Apart from that, I like the effect, it's more natural than having that blocky background, but it needs to be tuned a bit.

@raghunayyar
Copy link
Member Author

@jancborchardt fixed all the issues, do you want to fine tune the gradient? cc @oparoz

@jancborchardt
Copy link
Member

@oparoz that problem should be fixed if @raghunayyar fixed the ellipsis and forcing the filename display to one line. Then the filename will be shown on the bottom of the fade where it’s enough contrast.

@oparoz
Copy link
Contributor

oparoz commented Nov 3, 2015

@raghunayyar I apologise, looking at your code, I realised some of my changes had been merged against the wrong branch. Can I ask you to rebase? Since the change is mainly about grouping all the HTML in a template, it should be easy for you to make the small adjustments.

@jancborchardt The problem is not fixed by the title expansion, because the screenshot I've posted is of the extended title. I haven't checked if the gradient expands with the title. If not, then it should. If yes, then the gradient should fade away a bit higher.

@jancborchardt
Copy link
Member

If there is a longer title, it is either visible in the detail view, or should at most show as a tooltip. It should not result in the name extending to more rows than one in the overview. Ok?

@oparoz
Copy link
Contributor

oparoz commented Nov 4, 2015

If there is a longer title, it is either visible in the detail view, or should at most show as a tooltip. It should not result in the name extending to more rows than one in the overview. Ok?

It's not the current behaviour, but could be addressed in a separate PR, yes.

@jancborchardt
Copy link
Member

Right, but then the issue you mention above should not be taken into account for this PR if we fix it in a different one anyway. ;)

@raghunayyar raghunayyar force-pushed the flickr-style-gradients branch from 42e8cc6 to 4b14bb2 Compare November 4, 2015 17:52
@raghunayyar
Copy link
Member Author

@oparoz I have rebased my branch, do you want to tweak the gradient? cc @jancborchardt

}

#gallery .item-container .image-label .title:hover {
#gallery .item-container .image-label .title:hover.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo

@oparoz
Copy link
Contributor

oparoz commented Nov 5, 2015

@raghunayyar - Thanks for the rebase. No need to take care of the gradient since it's been decided to go with a tooltip.
I'm having some issues with the re-ordering of the HTML elements, so I first need to try and fix it before being able to merge this.

@jancborchardt
Copy link
Member

@raghunayyar also please squash your commits into one.

@oparoz
Copy link
Contributor

oparoz commented Nov 5, 2015

If someone wants to look at other ways of doing this.
The problem we're having is that when the label covers the whole surface, then it blocks the click handler. Because of old IE support, we can't use pointer-events and need to bubble down the event or place the label inside the anchor. That change is not compatible with the introducton of new features, so the structure should stay the same.

@jancborchardt
Copy link
Member

Why don’t we have the click handler on the label?

@oparoz
Copy link
Contributor

oparoz commented Nov 5, 2015

The container would probably be more appropriate, now that we have one.

@oparoz
Copy link
Contributor

oparoz commented Nov 5, 2015

But it has to be created from scratch since we were simply using anchors

@raghunayyar raghunayyar force-pushed the flickr-style-gradients branch from 4b14bb2 to 41e1478 Compare November 6, 2015 14:23
@raghunayyar raghunayyar force-pushed the flickr-style-gradients branch from 41e1478 to b87aaf1 Compare November 6, 2015 14:29
@raghunayyar
Copy link
Member Author

Can we merge this since all the discussed stuff has been fixed.

@oparoz
Copy link
Contributor

oparoz commented Nov 6, 2015

Not with the change of element order as it would need to be reverted in the future.

What we can do is wait for a PR changing the handler to be merged before merging this one.

@jancborchardt
Copy link
Member

So what’s the plan of action here so we move forward @oparoz @raghunayyar?

@oparoz
Copy link
Contributor

oparoz commented Nov 13, 2015

  1. Add a click handler to the container in this PR or another one which needs to be merged before this one
  2. Restore the order of elements in this PR
  3. Add a tooltip preferably in this PR or before merging this one so that we don't forget

@jancborchardt
Copy link
Member

@raghunayyar can you implement what @oparoz described above then? :)

@raghunayyar
Copy link
Member Author

@jancborchardt @oparoz I am occupied with calendar at the moment, can you take care, please?

setnes added a commit to setnes/gallery that referenced this pull request Dec 14, 2015
Renamed click handler for albums. (_openAlbum)
Cherry picked label gradients from owncloud#456.
Refactored links so they are not hidden by other elements.
setnes added a commit to setnes/gallery that referenced this pull request Dec 14, 2015
Added click handler for images. (_openImage)
Renamed click handler for albums. (_openAlbum)
Cherry picked label gradients from owncloud#456.
Refactored links so they are not hidden by other elements.
setnes added a commit to setnes/gallery that referenced this pull request Jan 16, 2016
Added click handler for images. (_openImage)
Renamed click handler for albums. (_openAlbum)
Cherry picked label gradients from owncloud#456.
Refactored links so they are not hidden by other elements.
setnes added a commit to setnes/gallery that referenced this pull request Jan 17, 2016
Added click handler for images. (_openImage)
Renamed click handler for albums. (_openAlbum)
Cherry picked label gradients from owncloud#456.
Refactored links so they are not hidden by other elements.
@oparoz oparoz mentioned this pull request Feb 11, 2016
8 tasks
@oparoz oparoz modified the milestones: 9.1-next, 9.0-current Feb 11, 2016
@oparoz
Copy link
Contributor

oparoz commented Feb 12, 2016

Closing as this has been added to the 9.1-next branch: ee073c9

@oparoz oparoz closed this Feb 12, 2016
@oparoz
Copy link
Contributor

oparoz commented Feb 12, 2016

Thank you for your contribution!

@jancborchardt jancborchardt deleted the flickr-style-gradients branch February 15, 2016 10:15
@jancborchardt
Copy link
Member

Nice!

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.

4 participants