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

Image Recommendations: Bottom Modal Sheet #4777

Merged
merged 33 commits into from Mar 21, 2024
Merged

Conversation

mazevedofs
Copy link
Collaborator

@mazevedofs mazevedofs commented Mar 12, 2024

Phabricator: https://phabricator.wikimedia.org/T356818

Notes

  • This PR adds the bottom sheet component for Image recommendations
  • On iPad and iPhone landscape, I'm considering an out-of-the-box approach for now until I get more input from design (Update: separate tickets were created for those items)
  • On the WKImageRecommendationBottomSheetView, in order to calculate the exclusion path to place the image on this almost columnar layout, it was necessary to calculate the size of the other view elements: the link button and the image view. I tried to keep the code as readable as possible, I'm open to suggestions here.
  • I'm not loading the Image on the image view, the image view is in gray color for now to help visualize the layout. Fetching, loading, and caching the image properly will be done in a separate ticket https://phabricator.wikimedia.org/T358933, since we need to build all the infrastructure for it

Test Steps

  1. Launch the app on Staging or Experimental (I've been testing with English and Spanish)
  2. Click the notifications button. You should see the modal sheet
  3. Make sure you can swipe up to make it bigger, but it can't be dismissed
  4. Test with larger fonts and other themes
  5. Click "Not sure". Verify that the next suggestion loads correctly
  6. Make sure you can scroll and reach the view article button on the article view
  7. On iPad - it shouldn't break
  8. The yes, no, and link buttons are not on the scope of this task, so they have no action

@mazevedofs mazevedofs requested review from a team and staykids and removed request for a team March 12, 2024 20:56
@mazevedofs mazevedofs added the Design Question(s) Tag @olga-ti label Mar 13, 2024
@mazevedofs mazevedofs marked this pull request as draft March 13, 2024 17:05
@mazevedofs mazevedofs marked this pull request as ready for review March 15, 2024 17:25
@mazevedofs mazevedofs removed the Design Question(s) Tag @olga-ti label Mar 15, 2024
@mazevedofs mazevedofs changed the title Image Recs - Bottom Modal Sheet Image Recommendations: Bottom Modal Sheet Mar 18, 2024
@tonisevener tonisevener requested review from tonisevener and removed request for staykids March 20, 2024 16:48
@tonisevener tonisevener self-assigned this Mar 20, 2024
Copy link
Collaborator

@tonisevener tonisevener left a comment

Choose a reason for hiding this comment

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

Looking good! I have mainly minor things. I have an alternative approach to avoid the sizing calculations you mentioned, but I didn't notice any layout bugs on how it's acting currently so it can be optional.

I also notice a crash with these steps:

  1. Go to first recommendation on iPhone (I've been testing iPhone 15 Pro simulator). Tap "View article"
  2. Go back, then tap "Not sure"
  3. App crashes.


private func setupToolbar() {
let spacer = UIBarButtonItem(systemItem: .flexibleSpace)
toolbar.setItems([yesToolbarButton, spacer, noToolbarButton, spacer, notSureToolbarButton], animated: true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see some constraint errors in the console when these are set, but so far I haven't figured out what's causing it.

@tonisevener tonisevener removed their assignment Mar 20, 2024
Copy link
Collaborator

@tonisevener tonisevener left a comment

Choose a reason for hiding this comment

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

We're going to merge to unblock dependent tasks. Feedback will be addressed in a followup PR.

@tonisevener tonisevener merged commit eda4703 into main Mar 21, 2024
4 checks passed
@tonisevener tonisevener deleted the half-sheet-modal-UI branch March 21, 2024 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants