-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Edit Comment: add initial view #17046
Conversation
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
You can trigger an installable build for these changes by visiting CircleCI here. |
Hey @mattmiklic . I wasn't sure if the new Edit Comment view should be a modal or not. The old one is, and I think that works well, at least on iPad. So I left it that way. But let me know if you think otherwise. |
Yep, that's correct. In the designs I'd mocked it up as a fullscreen modal rather than a sheet, but there's functionally not a big difference and this should be fine. |
Thanks @mattmiklic .
Yea, I totally didn't get fullscreen vs sheet 🤦. Sorry! I'll make that change in the next PR. |
No problem; thanks! |
Since @dvdchr hasn't reviewed this yet, I went ahead and updated it to show the new edit view in a full screen modal. Note the nav bar background matches what the app uses, not what's in Zeplin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on iPhone and iPad. Everything looks good! 👍
- Go to My Site > Comments > Comment details > Edit. Verify the new edit view is displayed.
- Go to Notifications > Comments > Notification details > Edit. Verify the new edit view is displayed.
Thanks @dvdchr ! |
Ref: #17000
This adds an initial new
Edit Comment
view.Cancel
andDone
buttons simply dismiss the view for now.newCommentEdit
feature flag, which is disabled by default.To test:
With
newCommentEdit
enabled:With
newCommentEdit
disabled:Regression Notes
Potential unintended areas of impact
N/A. Feature is incomplete and disabled.
What I did to test those areas of impact (or what existing automated tests I relied on)
N/A. Feature is incomplete and disabled.
What automated tests I added (or what prevented me from doing so)
N/A. Feature is incomplete and disabled.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.