-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
drop unneeded code #9245
drop unneeded code #9245
Conversation
Code looks like good changes, didn’t test yet. Btw, using an id called »save« seems stupid because way too generic. What about »#add-to-your-owncloud« or something descriptive like that? |
The inspection completed: 1 new issues |
🚀 Test Passed. 🚀 |
cc @owncloud/designers for review. |
@MorrisJobke wording change is good, but the confirm icon doesn’t seem to be there. |
@jancborchardt As I wrote in the comment: I can't get it work and added my changeset, which just works in firefox and chrome, but none of the IE versions. |
Is this PR already mergable ? It doesn't need to fix everything at once... |
I would say yeah, it looks good. 👍 @MorrisJobke ah, got it. That’s strange – how is that different from the other icons? Maybe it needs to be inside a div class="button" instead of an input? |
@PVince81 Yes it is. I just added this as info. |
So can we have a second review? This strikes off one issue from the server-server sharing ux review. |
👍 from me |
ref #8934
Following diff adds the icon-confirm button, but this just works on Chromium, Firefox and IE9. The fix for IE10 and IE11 is to apply the same fix as for IE9, but there are no easy ways to detect (except JS magic) IE10 and IE11. IE8 is broken.
@jancborchardt we need to fix the button CSS and header CSS. It's quite unstable ...