-
Notifications
You must be signed in to change notification settings - Fork 65
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
Conversation
agreed. white background without border should work 👍 |
I like this 👍 |
... and tested it of course :) |
👎 |
@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. |
8152136
to
1a6959e
Compare
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. |
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 ;-) |
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) 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. |
i don't have many white with transparent pictures because I use a colored transparency to better see, if transparency working. So it would be a bit odd, to make exactly THESE owncloud core pictures completely unviewable by new official gallery. |
@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. |
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). |
@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. 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
SolutionsButton to switch background+
-
White background + hover+
-
Pick the background colour from a thin ruler+
-
Automatic luminance+
-
Make button optional + white background + hover+
-
|
Most of these solutions were already discussed here: |
@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.. ? |
@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. 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. |
@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. |
That is, only the format of the image should have white background. The overall background of the slideshow should always be dark of course. |
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. |
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. |
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)) |
@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 …) |
I'll teach you! :P , but we're talking about CSS here. A checkerboard defined in :hover if I'm not mistaken.
Just like native SVG support is optional, you add one line to the config file and that enables the button |
Interestingly, the new right-sidebar is having the exact same problem the slideshow has: |
Closing this as the button is now optional. We need a new PR for the hover. |
Cool, thank you! :) |
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