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

Bugfix FXIOS-10574 Protection panel tweaks Design QA #23263

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

petruSt24
Copy link
Collaborator

📜 Tickets

Jira ticket
Github issue

💡 Description

Updated the design of the protection panel based on the feedback from design

📝 Checklist

You have to check all boxes before merging

  • Filled in the above information (tickets numbers and description of your work)
  • Updated the PR name to follow our PR naming guidelines
  • Wrote unit tests and/or ensured the tests suite is passing
  • When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
  • If needed, I updated documentation / comments for complex code and public methods
  • If needed, added a backport comment (example @Mergifyio backport release/v120)

Copy link
Collaborator

@MattLichtenstein MattLichtenstein left a comment

Choose a reason for hiding this comment

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

I noticed a few outstanding UI discrepancies between the current implementation and the spec. Are these supposed to accounted for?

Actual Expected
Favicon background is a rounded rectangle Favicon background should be circlular
The website title is bolded The website title should not be bolded
Spacing + centering of text inside the box with the fox image is incorrect Spacing between labels should be less, and content should be vertically centered
Privacy settings link has less leading margin Privacy settings link should have equal leading margin as the "Clear cookies and site data" button above it

@petruSt24
Copy link
Collaborator Author

Simulator Screenshot - iPhone 15 - 2024-11-21 at 21 00 32
@MattLichtenstein thank you for the suggestions, I replaced the stackView with a UIView to fix the spacing issue and updated the margin for the settings link button, but the website title is not bolded and it uses the menu navigation bar, they should look the same

@MattLichtenstein
Copy link
Collaborator

@petruSt24 it looks like we are changing the font of the HeaderView title from body to headline in HeaderView::setupDetails. Seems like this, and the corner radius of the favicon are artifacts of the old tracking protection panel and seem to need updating

Copy link
Collaborator

@MattLichtenstein MattLichtenstein left a comment

Choose a reason for hiding this comment

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

The connection details title and status text also do not seem to be centered when one of the labels are taking up more than one line
Simulator Screenshot - iPhone 16 - 2024-11-22 at 11 25 12

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.

2 participants