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

Add a button to toggle the colour of the background in the slideshow #40

Closed
3 of 4 tasks
oparoz opened this issue Feb 24, 2015 · 26 comments
Closed
3 of 4 tasks

Add a button to toggle the colour of the background in the slideshow #40

oparoz opened this issue Feb 24, 2015 · 26 comments

Comments

@oparoz
Copy link
Contributor

oparoz commented Feb 24, 2015

As described here by @anarcat
owncloud/core#11418 (comment)

Happens as soon as you're starting to preview more than just JPEGs.

Requirements

  • Design a toggle button. I don't see which standard oC icon we can use. Suggestions welcome.
  • Place the button next to the download button
  • Make the controls work with a light background. A solution could be to create a light theme Make the slideshow controls work with a light background #51
  • Implement the CSS background colour decoder and the switching logic

Open discussion

  • Should the user-chosen "theme" stay for as long as the app remains running? The colour of the slideshow won't change until the button is pressed again.
  • Should the theme be reset at every picture change?
  • Should the theme be reset every time the slideshow is launched?

Your input (and help) is welcome @jancborchardt, @anarcat

@anarcat
Copy link

anarcat commented Feb 24, 2015

Hmm... i am not sure that will solve the problem i described in #11418.

i believe it would be best to use a gray background like is done in the eog image viewer. it has worked well for me in general, as it is pretty rare that gray lines are drawn upon a transparent background. in such a case, maybe a background toggle would be useful, but it shouldn't flip to black either, maybe a darker gray, that's it.

my understanding of the situation is that the image rendering is unreliable: it doesn't yield consistent results. Sometimes the background is black, sometimes it's white, sometimes it's checkered. this needs to be made more consistent and reliable.

@jancborchardt
Copy link
Member

Yeah, this seems an over-the-top solution. We don’t need a »theming« toggle here.

@oparoz maybe have white as the background color for every image (only the image dimensions, not the whole background!) so that images with transparency will have white background?

Granted, this still doesn’t work for transparent images with light elements, but arguably those are more rare – and then the checkerboard pattern would still be shown on hover.

cc @owncloud/designers @icewind1991

@oparoz
Copy link
Contributor Author

oparoz commented Feb 24, 2015

@anarcat - This is to solve the problem highlighted in your comment, for full screen previews. Image rendering works, transparency is respected, but with a black background, you can't see anything when the picture is dark.
In my tests, I toggle between dark grey and black. Anything lighter, like the background in eog won't work with the current controls and that's the reason I want a light theme.
@jancborchardt - True, a picture sized light background could help. I'm going to see if it's compatible with Bigshot. That may re-introduce the checkboard pattern as well.

@oparoz
Copy link
Contributor Author

oparoz commented Feb 25, 2015

@jancborchardt - I've just played with a picture sized light background and it just looks weird. Maybe you have examples of apps which use this technique? Maybe there needs to be a border, etc.
Also, this plan fails as soon as you've got a picture with the same aspect ratio as the window. When that happens, the light background fills the viewport and we can't see the controls.

@jancborchardt
Copy link
Member

@oparoz what do you mean by »it just looks weird«? Can you post screenshots of how it looked like?

For the controls, we need to fix the visibility anyway for when bright images take up the whole viewport – that’s a different issue.

@oparoz
Copy link
Contributor Author

oparoz commented Feb 25, 2015

Screenshot

@jancborchardt
Copy link
Member

Well, that certainly looks less weird than just not seeing it. ;) But yeah, try adding a ~10px additional white border around it.

@oparoz
Copy link
Contributor Author

oparoz commented Feb 28, 2015

Anything less than 50px looks weird, but objects are no longer aligned and Bigshot is in control of centring, so looks like another div would need to be introduced.

@jancborchardt
Copy link
Member

@oparoz try adding box-sizing: border-box; to the container element. That should add the border around it, instead of making it shift the container to the right.

@oparoz
Copy link
Contributor Author

oparoz commented Mar 2, 2015

@jancborchardt - That worked, thanks :)

@oparoz
Copy link
Contributor Author

oparoz commented Mar 2, 2015

That creates weird side effects when zooming. The picture will end up being squeezed between the border when zooming out.

@jancborchardt
Copy link
Member

@oparoz can you supply a screenshot again please? :)

@oparoz
Copy link
Contributor Author

oparoz commented Mar 3, 2015

You won't be able to see it properly on a screenshot. I need top commit this change so that people can play with it

@oparoz
Copy link
Contributor Author

oparoz commented Mar 3, 2015

Updated master so that people can play with it and provide feedback. Let me know what you think @anarcat .

@jancborchardt - You can see one of the problems on the demo
https://oc8demo.interfacloud.com/index.php/apps/galleryplus/s/4jjwPv5E21l8Lnk
Zoom out on the SVG planet and you will make it disappear. If the original is very small, like an icon, it ends up being distorted.

@oparoz
Copy link
Contributor Author

oparoz commented Mar 4, 2015

@jancborchardt - Zoom is fixed via f12b14d

@jancborchardt
Copy link
Member

@oparoz ok, so no problems left? I can see no issues using the demo. :)

@jancborchardt
Copy link
Member

Btw, great on having the download action in the overview! Can you also add sharing, and favoriting? Just needs some styling then (putting it on the bottom, and making it look nice.)

And I would remove the »toggle background« function, it seems really overkill. We can still add it when we see that we really need it.

@oparoz
Copy link
Contributor Author

oparoz commented Mar 8, 2015

There is one visual problem, but I don't know if it's really bothering or not. I you load one of the wide pictures, you don't get the white border on the sides.

Regarding the toggle button. I've found myself using it quite a few times, especially on large photos. It's less bothering to have a black background. I was thinking of stealing something I saw on a site though, where they had something like 10 different tones side by side and you would just pick one. It would fit nicely in the top left corner I think.

For the download button, you can thank @libasys for the idea and example implementation :)

I've added a ticket for adding sharing in the slideshow, but it might look ugly with the default dropdown... and I haven't started to look at favourites yet

@deMattin
Copy link

Open discussion

  • Should the user-chosen "theme" stay for as long as the app remains running? The colour of the
    slideshow won't change until the button is pressed again.

I definitly vote for a setting, that remains until next switch.
It would even be best to save this preset and don't forget it on end of session.
Gallery of my OC is mostly used for "real" pictures (photos) by the users and you have to switch every time to "black" border. Because for photos black is the best setting and the white frame doesn't look nice.

There are definitly a lot less pictures, that need the white frame, than pictures where the white frame looks annoying.
Also my transparent pictures often need black frames, because they are designed to look good on dark backgrounds.

And so for now (if no better solution is found), the default frame colour should be black and not white.

@oparoz
Copy link
Contributor Author

oparoz commented Mar 20, 2015

This could be solved by #84 because there would rarely be a need to use the button and it would be just for the current picture.

@deMattin
Copy link

Am I able to hack the code meanwhile to get black as default?

As I described, it's temporary a better solution to switch to black as default ...

@oparoz
Copy link
Contributor Author

oparoz commented Mar 20, 2015

Yeah, piece of cake
https://github.com/interfasys/galleryplus/blob/master/js/slideshow.js#L248

Try a universal #363636. It's a grey which works with dark transparent pictures.

@deMattin
Copy link

For transparent pictures this works.
But the frame is still white!

Edit: Ok, I found it ... ;)
Thanks!
Looks good with: #363636

@oparoz oparoz mentioned this issue Mar 22, 2015
7 tasks
@oparoz
Copy link
Contributor Author

oparoz commented Mar 31, 2015

The default for JPGs is now black since there is no transparency.

@deMattin
Copy link

Very good idea to make it dependend on mime-type.
Now without a visual frame the jpg fullscreen view looks like it should!
Thanks!

@oparoz oparoz added this to the 8.2-current milestone Aug 31, 2015
@oparoz
Copy link
Contributor Author

oparoz commented Sep 8, 2015

Closing since no more work will be done on this.
Once a new way forward has been decided for users who store more than images, then I'll decide whether or not the button needs to become an option.

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

No branches or pull requests

4 participants