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

remove background toggle as it was very confusing for people #312

Closed
wants to merge 1 commit into from

Conversation

jancborchardt
Copy link
Member

Remove the background toggle button since it came up in many user tests it was very confusing. cc @karlitschek @tomneedham @oparoz

Instead, all images should have white background by default – without border! That way we will take care of nearly all transparent pngs/svgs. We can gradually improve to also fix the corner case of transparent images with white foreground.

Please review @owncloud/designers @oparoz

@karlitschek
Copy link

agreed. white background without border should work 👍

@MorrisJobke
Copy link
Contributor

I like this 👍

@MorrisJobke
Copy link
Contributor

I like this 👍

... and tested it of course :)

@oparoz
Copy link
Contributor

oparoz commented Sep 8, 2015

👎
That's not the proper way to do this. It should remain optional for users who actually rely on this because they can't see their images in the slideshow because they have a transparent background.

@oparoz
Copy link
Contributor

oparoz commented Sep 8, 2015

@jancborchardt - In order to get rid of it quickly, so that you don't have to add the setting, maybe just comment out the button and I'll make it optional later.

@jancborchardt jancborchardt force-pushed the remove-background-toggle branch from 8152136 to 1a6959e Compare September 8, 2015 13:46
@jancborchardt
Copy link
Member Author

can't see their images in the slideshow because they have a transparent background.

As we talked about – adding a white background by default will take care of nearly all transparent images, except the ones with white foreground. This is basically the same as the current approach except it’s automatic.

And for images with white foreground it will be fixed by adding something like showing checkerboard pattern or black background on hover. Again, we talked about this. We need to move in small steps here and having a very prominent interface element is not an elegant solution here.

@jospoortvliet
Copy link

I support the idea of first fixing the 95% use case before addressing the rest... And that's coming from somebody who has plenty of images which are white-on-transparent ;-)

@oparoz
Copy link
Contributor

oparoz commented Sep 8, 2015

The problem with merging incomplete features is that it takes forever to get the missing part done. I'd rather wait for the full PR since nothing really depends on this piece of work. On top of that there can always be unforeseen side-effects and reverting later on makes things more complicated.

I do agree that a button with no design is not elegant, but it could easily be replaced with a small colour scale letting the user pick the background he wants to use if black doesn't work. It does add some bright shades to the interface, but then no solution is perfect, apart from (#84)
The hover solution does have problems if you're trying to go through an album containing dark designs with a transparent background (icons, gifs, etc.). You don't want to have to hover over all of them and the checkboard does not really help.

So, getting rid of the button, yes, but we need a working alternative, in this PR, unless we go for optional button or auto-luminance.

@deMattin
Copy link

deMattin commented Sep 8, 2015

i don't have many white with transparent pictures because I use a colored transparency to better see, if transparency working.
But most of mixed white and transparent pictures are modificated ownclud pics!

So it would be a bit odd, to make exactly THESE owncloud core pictures completely unviewable by new official gallery.

@jospoortvliet
Copy link

@oparoz I think you are letting 'perfect' stand in the way of 'good enough'...

A full background IS good enough for almost everything. The few cases where it isn't - sure, that should be fixed some day but I wouldn't lose sleep over it if it wasn't. Meanwhile, right now - we can solve it for most cases.

@deMattin
Copy link

deMattin commented Sep 8, 2015

If there has to be something done NOW,without having a good solution, the checkerboard should be used as background. This way it's leastwise useable for any pictures (but not looking well in any case).
And I don't understand, why you say here, that white background fits most use cases as transparent background.
My pictures with transparency from other sources have mostly grey, white or black as non-transparent colors.
So a dark or light grey (or any color) would probably be a better choice, if you have to choose a color and no checkerboard.
Not switchable and fixed white or black for transparency background are the worst colors that can be choosen!

@oparoz
Copy link
Contributor

oparoz commented Sep 8, 2015

@jospoortvliet - The problem is that your 95% is not my 95% :). I was shocked when I first started to use ownCloud. Pictures app would only work with the Photos folder and would only show images of 3 media types. That does not work at all for SMEs, but it is perfectly fine for users dumping their phone's content to ownCloud.
All this to say, that it's usually best to fix the design rather than remove the feature, unless it's not in line with the USP, chart, etc. in which case Gallery+ can take over.

I have implemented most of the new features for a reason. There is a need for them. Not by me, but by people using the system daily and who run into those issues. In a few of those merged PRs, there is a visual design problem, no question about it (this button is simply the old file icon!). That's because that's not my trade and few people have stepped in to help me.

@deMattin - In my initial tests, I did pick a grey, the colour is in the comments, but I think it wasn't deemed to be an acceptable solution.

Action plan

  1. The data sample used for testing should be seriously augmented with real life material. 3 JPGs doesn't cut it
  2. Test all/most solutions
  3. Pick a solution

Solutions

Button to switch background

+

  • works in 100% of the cases

-

  • not pretty
  • distracting
  • needs manual intervention

White background + hover

+

  • works in 53.45%-95% of the cases

-

  • Discovery issue
  • needs manual intervention
  • needs to keep the finger on the picture to be able to see it

Pick the background colour from a thin ruler

+

  • works in 100% of the cases

-

  • needs manual intervention
  • distracting

Automatic luminance

+

  • works in 99% of the cases

-

  • doesn't work with SVGs, which are not supported anyway

Make button optional + white background + hover

+

  • works in 100% of the cases

-

  • needs documentation so that people needing the feature can enable it
  • won't be pretty for people enabling the feature ;(

@oparoz
Copy link
Contributor

oparoz commented Sep 9, 2015

Most of these solutions were already discussed here:
#226

@jospoortvliet
Copy link

@oparoz IF you can detect transparency, use a checkerboard for those and black for the rest... This can even be based on file type - 90% of the photo's are JPG, no transparency there - black back. GIF and PNG can have transparence - checkerboard.. ?

@oparoz
Copy link
Contributor

oparoz commented Sep 9, 2015

@jospoortvliet - Yes, I already pick the proper background based on the media type, but it's currently white and doesn't work for some files, so people still use the button.
Using a checkerboard pattern would be better, but it's not a silver bullet as @deMattin has shown:
#226 (comment)

The slideshow is a viewing tool and should work in all situations or it will lead to frustration.

Until auto-luminance can be implemented and if no decision is made before the deadline, I'll probably make the button optional and use a pretty brightness icon.
There is still the issue of the border. Without it border-to-border pictures don't look good, but that could be considered an edge case.

@jancborchardt
Copy link
Member Author

IF you can detect transparency, use a checkerboard for those and black for the rest

@jospoortvliet you mean white background for the rest, right? Because any normal editor like GIMP, Inkscape (yes also the Adobe ones) use white as default background.

@jancborchardt
Copy link
Member Author

That is, only the format of the image should have white background. The overall background of the slideshow should always be dark of course.

@oparoz
Copy link
Contributor

oparoz commented Sep 9, 2015

Well in bitmap editors, if you work with transparent images, you use the transparent background, which is represented by a checkboard. It's true that Inkscape uses a white background as default, but I'm sure this can be easily changed when dealing with white objects.

@jancborchardt
Copy link
Member Author

It's true that Inkscape uses a white background as default, but I'm sure this can be easily changed when dealing with white objects.

It’s actually not that easy – you have to go into the settings and manually change the background. But let’s not take that as a good example.

We’re running in circles here: Default background should be white (only for the image size of course, so not visible for jpgs etc), and checkerboard on hover. This will take care of most cases. When that is done we can use that as a base for further improvements.

@oparoz
Copy link
Contributor

oparoz commented Sep 9, 2015

Default background should be white (only for the image size of course, so not visible for jpgs etc), and checkerboard on hover.

OK, so please create a PR with just that. Then we can work on making the button optional and the final item on the list would be how to deal with border-to-border images (#40 (comment))

@jancborchardt
Copy link
Member Author

@oparoz I don’t have enough JS knowledge to do that as you know. Would be cool if you can take care of it.

What would mean making the button optional? A config switch somewhere in a text file? (That would be fine by me, as with that implementation mentioned above I really think most cases will be covered …)

@oparoz
Copy link
Contributor

oparoz commented Sep 10, 2015

@oparoz I don’t have enough JS knowledge to do that as you know

I'll teach you! :P , but we're talking about CSS here. A checkerboard defined in :hover if I'm not mistaken.
Actually, this should be a junior job for someone interested in the slideshow... @tomneedham @raghunayyar @setnes maybe?

What would mean making the button optional?

Just like native SVG support is optional, you add one line to the config file and that enables the button

@oparoz
Copy link
Contributor

oparoz commented Sep 10, 2015

Interestingly, the new right-sidebar is having the exact same problem the slideshow has:
owncloud/core#18796 (comment)

@oparoz
Copy link
Contributor

oparoz commented Sep 13, 2015

Closing this as the button is now optional. We need a new PR for the hover.

@oparoz oparoz closed this Sep 13, 2015
@MorrisJobke MorrisJobke deleted the remove-background-toggle branch September 13, 2015 08:51
@jancborchardt
Copy link
Member Author

Cool, thank you! :)

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.

6 participants