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 download/available offline icons #2152

Merged
merged 7 commits into from
May 28, 2018
Merged

fix download/available offline icons #2152

merged 7 commits into from
May 28, 2018

Conversation

theScrabi
Copy link
Contributor

@theScrabi theScrabi commented Mar 15, 2018

fixes #2145 and #2081


BUGS & IMPROVEMENTS

@davigonz
Copy link
Contributor

davigonz commented Apr 11, 2018

@theScrabi Do we really need download_pin.svg.png and offline_pin.svg.png? The svg could be useful but those png are not being used while downloaded_pin.png and offline_available_pin.png do.

@theScrabi
Copy link
Contributor Author

Do we really need download_pin.svg.png

No, that happened by exident.

@@ -55,16 +55,7 @@
android:layout_marginTop="4dp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why downloaded_pin, error_pin, offline_available_pin and sync_ping have these margins and would they be properly adapted to all the resolutions without putting them in mdpi, xhdpi and so on?

screen shot 2018-05-11 at 08 57 33

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I honestly don't know, it just was that way before so I left it.

android:layout_marginBottom="4dp"
android:layout_marginRight="4dp"
android:src="@drawable/ic_available_offline" />
android:src="@drawable/sync_pin" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we are replacing ic_available_offline with sync_pin, is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because the image set here is just a place holder. The actual image gets set dynamically.

android:layout_marginBottom="2dp"
android:layout_marginRight="2dp"
android:src="@drawable/ic_available_offline" />
android:src="@drawable/sync_pin" />
Copy link
Contributor

Choose a reason for hiding this comment

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

And here ic_available_offline with sync_pin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here

@davigonz
Copy link
Contributor

davigonz commented May 11, 2018

@theScrabi The code looks good for me but please delete download_pin.svg.png and offline_pin.svg.png since they are not needed.

@davigonz
Copy link
Contributor

Changes approved, ready to test

@jesmrec
Copy link
Collaborator

jesmrec commented May 17, 2018

(1)

  1. Set a folder as av. offline

Current: Once all the stuff info the folder is marked with the pink tick, the folder itself and its subfolders aren't

Expected: If a folder is set as av. offline, the av. offline mark is also set in the folder icon

Tested with Nexus 6P

@theScrabi
Copy link
Contributor Author

@jesmrec that should be fixed.

@jesmrec
Copy link
Collaborator

jesmrec commented May 24, 2018

(2)

Steps:

  1. Set a folder with subfolders as available offline

Current: subfolders does not have the icons
Expected: All content into an available offline folder has the icon

@theScrabi
Copy link
Contributor Author

fixed issue for sub folder.

@jesmrec
Copy link
Collaborator

jesmrec commented May 25, 2018

Maybe we need here a bit of discussion, so i realised that we are losing one state.

Before, the available offline mark was set on the bottom right of the icon, so in the top we set the "syncing" icon to show the user that the file is being synced at this moment:

gif1

Right now, when the icon is set, no status is shown:

gif2

so, we know that the item is av. offline, but not if it is being synced.

Options to fix:

  • Set the sync option on the bottom right or in the center of the icon, and the top right to the downloaded/av. offline mark
  • Overlap the av. offline with the sync icon, as the downloaded icon does (do not like this option)
  • Let it be, and lose the "av. offline currently syncing" state.

any other ideas will be welcome.

@davigonz @michaelstingl

@theScrabi
Copy link
Contributor Author

@jesmrec :
I fixed the problem that the available offline pin overrides the sync pin, also I put the pin to the bottom.
Additionally I increased the size of the pins for grid view.

screenshot_1527496376 screenshot_1527496393
screenshot_1527496416

@jesmrec
Copy link
Collaborator

jesmrec commented May 28, 2018

Approved

@davigonz davigonz merged commit 6dfc375 into master May 28, 2018
@davigonz davigonz deleted the downicon branch May 28, 2018 12:45
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.

New downloaded/offline available icons + pins
3 participants