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

Fix for #28 #38

Merged
merged 7 commits into from
Apr 7, 2019
Merged

Fix for #28 #38

merged 7 commits into from
Apr 7, 2019

Conversation

sivaraam
Copy link
Member

This PR includes some changes to fix the issues noted in #28. Apart from that it adds a site icon which is just the app's icon as of now.

Fixes #28.

The numbering of the list items seems very large when compared to the
size of the list item. This results in the numbers looking a little
odd.

So, reduce the size of the list item numbers.
This helps save some unneeded wastage of space when showing the
QR code.

Only white space in the border has been trimmed no other consequence
intended.
This helps save some bandwidth and helps with the quicker loading of
the website possibly without losing anything else.
This helps ensure that the QR code is center aligned in smaller
screens.
Grouping the buttons helps with fixing the issue that they
are aligned closer to each other on smaller screens.

For the same reason, the buttons are not qualified as 'btn-lg'.
This icon is just the app's icon taken from the app's source.

Location of app icon:
apps-android-commons/app/src/main/res/mipmap-xhdpi/ic_launcher.png

Commit: 69b0f4035 (Fix #2793: Campaigns work on Beta (#2798), 2019-03-30)
Copy link
Member

@domdomegg domdomegg left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Maybe one suggestion would be to keep the QR code as PNG as JPEG results in aliasing artifacts. Using something like tinypng will massively reduce its filesize down to less than jpeg actually, to 17KB. Ideally (maybe for the future) we could either use SVG or reconsider even it's need there.

@domdomegg
Copy link
Member

In case it helps, here's the compressed png:

commons-app-qr

This is to avoid aliasing effect in the image as mentioned by
@domdomegg.
@sivaraam
Copy link
Member Author

sivaraam commented Apr 7, 2019

Thanks a lot for the tip about using compressed PNG @domdomegg and mentioning about the drawback of JPEG.

I've just replaced the JPEG version with the PNG you gave here (thanks for that too!). We'll come back to re-considering/using an SVG version it later :)

@sivaraam sivaraam merged commit eb865f2 into commons-app:master Apr 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revamp the layout of the website
2 participants