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

Talk pages - banner design review feedback #4384

Merged
merged 6 commits into from Nov 3, 2022

Conversation

tonisevener
Copy link
Collaborator

Phabricator:
https://phabricator.wikimedia.org/T312314#8350922
https://phabricator.wikimedia.org/T311639#8280839

Notes

These are design review tweaks for the alerts presented on talk pages. They dipped into wider-reaching changes in e139b2e and 9997c9a. One is to add a shadow to all banners, which would be easy to revert if design decides against it. The other is reworking the presentation constraints and view controller picking to better consider safe area. I could not figure out a low-touch way around some of the layout bugs we were seeing, and this probably is more stable approach in the medium-term (though, I think we should rewrite it entirely in the long term). I tested a few banners throughout the app and haven't noticed any glaring issues. I'll also call this change out in the tickets so that QA can keep an eye out for regressions here.

Test Steps

  1. Confirm subscription banner text is no longer blue for the light theme.
  2. Confirm there's some bottom padding with subscription banners on devices that have a home button (I tested on iPhone 7).
  3. Confirm you now see a bottom shadow on top banners, and a top shadow on bottom banners.

@tonisevener tonisevener changed the title Talk pages/banners design review 2 Talk pages - design review on banners Nov 2, 2022
@tonisevener tonisevener changed the title Talk pages - design review on banners Talk pages - banner design review tweaks Nov 2, 2022
@tonisevener tonisevener changed the title Talk pages - banner design review tweaks Talk pages - banner design review feedback Nov 2, 2022
Copy link
Collaborator

@mazevedofs mazevedofs left a comment

Choose a reason for hiding this comment

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

This is looking great, the weird safety area layout spacing is gone! Just one question, in the sepia theme the banner has the default coloring, is this intended? (It is like this in main, so I'm just double checking)

Simulator Screen Shot - iPhone 14 Pro - 2022-11-03 at 12 39 42

@mazevedofs mazevedofs merged commit 7750d44 into main Nov 3, 2022
@mazevedofs mazevedofs deleted the talk-pages/banners-design-review-2 branch November 3, 2022 16:23
@tonisevener
Copy link
Collaborator Author

This is looking great, the weird safety area layout spacing is gone! Just one question, in the sepia theme the banner has the default coloring, is this intended? (It is like this in main, so I'm just double checking)

Simulator Screen Shot - iPhone 14 Pro - 2022-11-03 at 12 39 42

Good catch on that - I'll ask! I think it's always been a white background for Sepia, strangely. But this seems like I good time to update it while we're here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants