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

Added AnkiWeb logo to 'AnkiWeb account' screen and 'Logged in' screen. #17204

Merged
merged 2 commits into from
Oct 9, 2024
Merged

Added AnkiWeb logo to 'AnkiWeb account' screen and 'Logged in' screen. #17204

merged 2 commits into from
Oct 9, 2024

Conversation

divyansh1576
Copy link
Contributor

@divyansh1576 divyansh1576 commented Oct 6, 2024 •
edited by david-allison
Loading

Purpose / Description

To change the Ankidroid logo to AnkiWeb logo on Login page and My Accounts screen .

Fixes

How Has This Been Tested?

Tested on Android physical device. ( API - 34)

Screenshot

6 7
My account My account
3 4
Login screen Login screen

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@welcome Welcome
Copy link

welcome bot commented Oct 6, 2024

First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible.

@david-allison
Copy link
Member

Love the concept!

The logos on the left and right should be larger with more margin between the star and the border

@thedroiddiv
Copy link
Member

Just a UI suggestion, when not logged in show the link as disconnected? And when logged in show it connected?

@snowtimeglass
Copy link
Contributor

Thank you for the PR.

I think it would be more desirable to minor-adjust the color of the dashed line and the chain, and the spacing of them. In other words, I personally prefer more consistency in the spacing and the color (or the gradation of the colors).

(image)
image

@david-allison
Copy link
Member

I'd love to see @snowtimeglass' suggestion, with a little more padding between the star and the border of the circle (likely by enlarging the circle, and keeping the star in the same position)

david-allison
david-allison requested changes Oct 7, 2024
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Moving the above comment on the logo change to be a blocker on merging

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Oct 7, 2024
@divyansh1576
Copy link
Contributor Author

Changes

  • Changed the login logo as per the suggestion of @snowtimeglass .
  • Added more padding between the logo and the border of Ankiweb.
  • Changed the preview in PR description.

@david-allison
Copy link
Member

david-allison commented Oct 7, 2024 •
edited
Loading

Much better! I love the coloring on the link

I'd make the circles slightly bigger still, but that's a nitpick

The blocker is that the logos aren't aligned vertically/the same size

Screenshot_20241007_181611_Firefox.jpg

@divyansh1576
Copy link
Contributor Author

Updated as per the suggestion

  • Increased the size of the circle.
  • Adjusted the size/alignment of the logo's.
    Screenshot 2024-10-08 002947

@david-allison
Copy link
Member

@divyansh1576 ping me if this isn't reviewed within 24 hours. I'm not currently at my PC

I suspect my feedback will be that the star on the right has been skewed by a scale operation

@snowtimeglass
Copy link
Contributor

Thank you for the change!

AnkiDroid icon looks good to me.

Because of the symmetry of the dot color gradation between the left side and right side, the dot on the right end is darker than AnkiWeb star, which rather looks inharmonious to me.
image

How about making the dot color gradation on the right side brighter than the color of AnkiWeb star?

(image)

image

@divyansh1576
Copy link
Contributor Author

How about making the dot color gradation on the right side brighter than the color of AnkiWeb star?

Thanks for the detailed review, Sounds good to me .

david-allison
david-allison approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Looks great, cheers!

@david-allison david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Author Reply Waiting for a reply from the original author labels Oct 9, 2024
@xenonnn4w
Copy link
Contributor

WhatsApp Image 2024-10-09 at 16 40 31_e8f47313
If possible, as per above discussion you can try having something like this for Log In screen.

@divyansh1576
Copy link
Contributor Author

@xenonnn4w Updated!

image

david-allison
david-allison approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

If I've approved and you haven't made significant changes, there's no need to ask for reapproval. Still good to go 😁

@mikehardy mikehardy added the squash-merge The pull request currently requires maintainers to "Squash Merge" label Oct 9, 2024
mikehardy
mikehardy approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@mikehardy mikehardy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is really cool! I like the way it looks

I tested it locally and found a problem with landscape mode where now that there are two logos (instead of just one), only one of the logos was hidden in landscape mode

Hiding the logo in landscape is intentional to preserve screen real estate for the actual interactive parts of the screen so I think it's worth maintaining, and with apologies it was easier for me to just play with the fix and commit it then to describe it, so I did that

Here's a video of the landscape switching in both logged in and logged out states now to show it is working

I'm +1 on it

ankilogo_with_landscape_logo_vis_fix.webm

(Note: there may be a quibble with the padding of "Logged in as..." in landscape in the logged in case but I don't think this PR touched that, should be a follow-on IMHO)

divyansh1576 and others added 2 commits October 9, 2024 22:07
@divyansh1576 @david-allison
Added AnkiWeb logo to 'AnkiWeb account' screen and 'Logged in' screen
2ef9e91 
This helps visually explain that AnkiWeb is a connected service, rather
 than part of AnkiDroid

* When not logged in, the link icon is disconnected
* The dot color gradient matches the star on the left & right side

Fixes 16300
@mikehardy @david-allison
fix: handle logged in logo visibility in landscape
9798e7b 
previously the two logos were assigned to the same variable,
so toggling visibility on landscape mode hid one of the logos
(the login one) but did not hide the other one (logged in one)

now we maintain reference to both logos and toggle visibility
of both in landscape mode
david-allison
david-allison approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Squashed & changed commit message.

Good to go

@david-allison david-allison removed the Needs Second Approval Has one approval, one more approval to merge label Oct 9, 2024
@david-allison david-allison added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed squash-merge The pull request currently requires maintainers to "Squash Merge" labels Oct 9, 2024
@david-allison david-allison added this pull request to the merge queue Oct 9, 2024
Merged via the queue into ankidroid:main with commit d189482 Oct 9, 2024
9 checks passed
@github-actions github-actions bot added this to the 2.19 release milestone Oct 9, 2024
@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Oct 9, 2024
@david-allison david-allison added the hacktoberfest-accepted Accepted for Hacktoberfest label Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-allison david-allison david-allison approved these changes

@mikehardy mikehardy mikehardy approved these changes

Assignees
No one assigned
Labels
hacktoberfest-accepted Accepted for Hacktoberfest New contributor
Projects
None yet
Milestone
2.19 release
Development

Successfully merging this pull request may close these issues.

[suggestion] Add AnkiWeb logo to 'AnkiWeb account' screen
6 participants
@divyansh1576 @david-allison @thedroiddiv @snowtimeglass @xenonnn4w @mikehardy