Skip to content

SpeedGrader comments #3348

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

Merged
merged 37 commits into from
May 30, 2025
Merged

SpeedGrader comments #3348

merged 37 commits into from
May 30, 2025

Conversation

rh12
Copy link
Contributor

@rh12 rh12 commented May 6, 2025

refs: MBL-18809
affects: Teacher
release note: none

What's changed

  • Updated SpeedGrader comment list to match new design (see specific Figma link in ticket)
  • Comment bubble & send button colors now match context color
  • Extracted related code to viewmodels, interactor, assembly, etc.
  • Fixed keyboard layout issues

Chores

  • Added scaledSize(), scaledIcon(), scaledFrame(), scaledOffset() view modifiers
  • Added convenience method sinkFailureOrValue()
  • Extracted various MockErrors to a central place

What's not changed

  • Please see ticket for detailed scope first
  • New comment is displayed as Sent instantly, regardless of the API response.
    • It was like this before, it probably needs a rework.
    • Even though I've created a SubmissionCommentsInteractor I didn't rework the create comment methods, just used them as is for now.
    • I haven't added tests for these
  • Attempts are still in the comments
  • The new Submission Comment related code is still in Teacher app, even though they could reused later in S-tudent app

Test Plan

  • Please read the above and the ticket for scope
  • Verify comments sections matches Figma (minus what's out of scope)
  • Verify Keyboard show (and hide) works properly and doesn't add big blank spaces
    • examples: comment, rubric comment, grade input, text annotation

Screenshots

BeforeAfter

Checklist

  • Follow-up e2e test ticket created
  • A11y checked
  • Tested on phone
  • Tested on tablet
  • Tested in dark mode
  • Tested in light mode
  • Approve from product

@rh12 rh12 self-assigned this May 6, 2025
@inst-danger
Copy link
Contributor

inst-danger commented May 6, 2025

Teacher Build QR Code:

@inst-danger
Copy link
Contributor

inst-danger commented May 6, 2025

Warnings
⚠️ This pull request will not generate a release note.

Affected Apps: Teacher

MBL-18809

Coverage New % Master % Delta
Canvas iOS 91.63% 91.6% 0.03%

Generated by 🚫 dangerJS against da197d2

Copy link
Contributor

@suhaibabsi-inst suhaibabsi-inst left a comment

Choose a reason for hiding this comment

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

Code +1
Few minor comments to consider.

@rh12 rh12 dismissed stale reviews from ndrsszsz and suhaibabsi-inst via b009b6e May 14, 2025 07:40
@rh12
Copy link
Contributor Author

rh12 commented May 14, 2025

Refactored scaledSize() method after discussion with @vargaat
Also fixed related issues, based on my misunderstanding of the icon sizes we use (for the record, the image itself already contains the bounding box)

@rh12 rh12 requested a review from vargaat May 14, 2025 07:43
@rh12
Copy link
Contributor Author

rh12 commented May 19, 2025

Waiting for UX approval, moving to draft for now

@rh12 rh12 marked this pull request as draft May 19, 2025 12:05
rh12 added 5 commits May 26, 2025 14:43
# Conflicts:
#	Teacher/Teacher.xcodeproj/project.pbxproj
#	Teacher/Teacher/SpeedGrader/Comments/ViewModel/SubmissionCommentListViewModel.swift
#	Teacher/Teacher/SpeedGrader/MainLayout/View/SubmissionGraderView.swift
#	Teacher/Teacher/SpeedGrader/MainLayout/ViewModel/SubmissionGraderViewModel.swift
#	Teacher/Teacher/SpeedGrader/StudentPager/ViewModel/SpeedGraderViewModel.swift
#	Teacher/TeacherTests/SpeedGrader/Comments/ViewModel/SubmissionCommentListViewModelTests.swift
@rh12 rh12 marked this pull request as ready for review May 27, 2025 10:41
@rh12
Copy link
Contributor Author

rh12 commented May 27, 2025

Opening the PR for review again.
The text field area UX had been adjusted to look like before the PR.
There are 3 variants in the code for now, this will be cleaned up in a followup task, once the final design is approved.

The textfield height calculation is not perfect and the size is limited based on the containers height, resulting in a somewhat shorter height than expected. (It was like this before as well)
These will be consolidated during the followup task.

Copy link
Contributor

@suhaibabsi-inst suhaibabsi-inst left a comment

Choose a reason for hiding this comment

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

Tested on iPhone XS, iOS 18.3.1

I found that there is a quite noticeable hitch when showing and dismiss keyboard, See below recording.

ScreenRecording_05-27-2025.16-16-03_1.MP4

Moreover, design of textfield on Figma file is different from that developed, where buttons of attachment and quick responses is laid below the text input itself.

Screenshot 2025-05-27 at 4 18 53 PM

Also, there a white space between the very bottom of the screen and the bottom of screen safeArea, which should be filled with the same color of the input back. The same way as in Tab and Tool Bars.

IMG_2258

@rh12
Copy link
Contributor Author

rh12 commented May 27, 2025

@suhaibabsi-inst

I found that there is a quite noticeable hitch when showing and dismiss keyboard, See below recording.

The video doesn't load, could you please upload it again?
If this is about the issue when on iPads in portrait mode the textfield focus reloads the screen that is being fix by Attila below

Moreover, design of textfield on Figma file is different from that developed, where buttons of attachment and quick responses is laid below the text input itself.

Yes, it had been stated that we use the old design for now, because the intended design waits for a11y approval.

Also, there a white space between the very bottom of the screen and the bottom of screen safeArea, which should be filled with the same color of the input back. The same way as in Tab and Tool Bars.

Yes, it's been there before, as well. But since the old design will go away I didn't fix this one.

rh12 added 2 commits May 28, 2025 11:35
# Conflicts:
#	Teacher/Teacher/Localizable.xcstrings
# Conflicts:
#	Teacher/Teacher/SpeedGrader/StudentPager/ViewModel/SpeedGraderViewModel.swift
vargaat
vargaat previously approved these changes May 28, 2025
Copy link
Collaborator

@vargaat vargaat left a comment

Choose a reason for hiding this comment

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

QA+1, Let's to some optimization at the end when we delivered all changes.

@rh12
Copy link
Contributor Author

rh12 commented May 29, 2025

Thanks @vargaat for the rotation fix! It works for me on iPhone @ 18 and iPad @ 17

@rh12 rh12 merged commit be2f7bc into master May 30, 2025
4 checks passed
@rh12 rh12 deleted the feature/MBL-18809-SpeedGrader-comments branch May 30, 2025 09:13
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.

5 participants