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.

Sign up for GitHub

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

Jump to bottom

Fix icons on App page in FF #905

Merged
merged 6 commits into from
Aug 17, 2016
Merged

Fix icons on App page in FF #905

merged 6 commits into from
Aug 17, 2016

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Aug 17, 2016 •
edited by MorrisJobke
Loading

For #900

  • Filter not working
  • No scaling

Once done:

  • Check Chrome
  • Check FF
  • Check IE/Edge
  • Check Safari
  • Check opera

@rullzer rullzer added the 2. developing Work in progress label Aug 17, 2016
@mention-bot
Copy link

@rullzer, thanks for your PR! By analyzing the annotation information on this pull request, we identified @raghunayyar, @DeepDiver1975 and @PVince81 to be potential reviewers

@MorrisJobke
Introduce weird hack for Firefox
1444c1d 
* everything is 4 times as big (doubled width and doubled height)
blizzz
blizzz reviewed Aug 17, 2016
View reviewed changes
settings/js/apps.js
@@ -234,6 +234,11 @@ OC.Settings.Apps = OC.Settings.Apps || {

imageUrl : function (url, appfromstore) {
var img = '<svg width="72" height="72" viewBox="0 0 72 72">';
// weird hack because in Firefox everything is 4 times as big (doubled in width and doubled in height)
// TODO: replace this with a proper solution
Copy link
Member

Choose a reason for hiding this comment

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

can we have todos as issues instead, pls?

Copy link
Member

Choose a reason for hiding this comment

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

I will create it

Copy link
Member

Choose a reason for hiding this comment

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

#908

@MorrisJobke MorrisJobke mentioned this pull request Aug 17, 2016
Closed
@rullzer
Copy link
Member Author

rullzer commented Aug 17, 2016

Fine by me for now 👍

rullzer added 2 commits August 17, 2016 15:16
@rullzer
Remove hack
314b400
@rullzer
Add viewBox to app icons
ba4633b 
If we don't add a viewbox a lot of browsers will just say NOPE!!! when
we ask them to scale.
@rullzer
Copy link
Member Author

rullzer commented Aug 17, 2016

It seems it is because a lot of icons are missing a viewBox attribute. So browsers just say sorry but I can't scale that.
I pushed some updated icons to test.

@MorrisJobke
Copy link
Member

MorrisJobke commented Aug 17, 2016 •
edited
Loading

I tested this now in Edge, IE11, IE10, Safari, Chrome, Firefox and it works just fine 👍

@MorrisJobke MorrisJobke added 3. to review Waiting for reviews backport-request and removed 2. developing Work in progress labels Aug 17, 2016
@MorrisJobke
Copy link
Member

@karlitschek We need this backported (and it would be best to have this in before the RC - looking towards @LukasReschke 😉 )

@MorrisJobke
Copy link
Member

I tested this with Opera too and it works there :)

@karlitschek
Copy link
Member

nice. please backport

@LukasReschke
Copy link
Member

LGTM and world doesn't explode 👍

@LukasReschke LukasReschke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 17, 2016
@MorrisJobke MorrisJobke mentioned this pull request Aug 17, 2016
Merged
7 tasks
@MorrisJobke MorrisJobke mentioned this pull request Aug 17, 2016
Merged
This was referenced Aug 17, 2016
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
@MorrisJobke
Copy link
Member

@karlitschek for all the backports above 🙈 It's mostly only adding a viewBox="0 0 32 32" to the SVGs because otherwise FF and IE doesn't scale SVGs at all. In other browsers this is the default behavior.

Then the app management looks nice for all shipped apps.

This was referenced Aug 17, 2016
Merged
Merged
Merged
Merged
GitHubUser4234 pushed a commit to GitHubUser4234/server that referenced this pull request Aug 30, 2016
@MorrisJobke
Merge pull request nextcloud#905 from nextcloud/invisible_icons
07bf33c 
Fix icons on App page in FF
@MorrisJobke MorrisJobke mentioned this pull request Sep 1, 2016
Merged
@MorrisJobke MorrisJobke removed their assignment Sep 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@rullzer rullzer

Labels
4. to release Ready to be released and/or waiting for tests to finish
Projects
None yet
Milestone
Nextcloud 11.0
Development

Successfully merging this pull request may close these issues.
6 participants
@rullzer @mention-bot @MorrisJobke @karlitschek @LukasReschke @blizzz