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

Comment Detail: Add UI for the delete button #17286

Merged
merged 4 commits into from
Oct 8, 2021

Conversation

dvdchr
Copy link
Contributor

@dvdchr dvdchr commented Oct 8, 2021

Refs #17087

This adds the UI for the "Delete Permanently" button. The functionality hasn't been implemented yet, so clicking on it does nothing. A couple of notes:

  • From the designs, note that there is no separator for the cell before the "Delete Permanently" button. To achieve this, I've alternatively explored viewForFooterInSection, or through tableView.tableFooterView and ended up registering it as a table view row.
    • For viewForFooterInSection in .plain table view style, the footer sticks to the bottom screen. To make sure that the footer stays at the bottom, I'd have to change the table style to grouped, but this introduces several bits like different cell background colors and additional padding around the top.
    • For tableFooterView, the container doesn't support auto layout (shocking), so I'd have to manually calculate the height of the footer view after the layout pass is completed (probably via systemLayoutFittingSize), and then resize the height of the tableFooterView.
    • Finally, if it's registered as a normal table view cell, I just need to hide the cell separator (via separatorInset) when the cell is placed right before the "Delete Permanently" button. I decided to go with this instead.
  • The current tableFooterView is used to hide the cell separator for the last row, but I've noticed that the "Delete Permanently" button is too close to the tab bar. Checking on the designs, I've noticed that there's 45pt of distance between the button and the tab bar, so I've added some height for the tableFooterView to implement the bottom padding.

Lastly, a tiny question for @mattmiklic: here's how the button is currently displayed and highlighted (both in Light and Dark mode). Does this look good for you? Thanks!

Light 🌝 Dark 🌚
Normal light_normal dark_normal
Highlighted light_highlighted dark_highlighted

To test

  • Ensure that the New Comment Detail flag is enabled.
  • Ensure that you have moderation rights for the selected site.
  • Go to My Site > Comments.
  • Select any comments in the Spam or Trashed filter.
  • Verify that the Delete Permanently button is shown.

Regression Notes

  1. Potential unintended areas of impact
    n/a. The feature is still hidden behind a flag.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    n/a. The feature is still hidden behind a flag.

  3. What automated tests I added (or what prevented me from doing so)
    n/a. The feature is still hidden behind a flag.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 8, 2021

You can trigger an installable build for these changes by visiting CircleCI here.

@dvdchr dvdchr added [Status] Needs Design Review A designer needs to sign off on the implemented design. and removed [Status] Needs Design Review A designer needs to sign off on the implemented design. labels Oct 8, 2021
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 8, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@mattmiklic
Copy link
Member

The delete buttons overall look good! I just want to double-check the red color we're using. When I sampled the color from the screenshot it looks a little different than Red 50 from Color Studio. Can you double-check that it is Red 50?

In Dark Mode, we should lighten the red by a shade to make the red text more legible on a dark background, so that should be Red 40.

I think the Color Studio reds are already mapped to the semantic .error color but if you could double-check I'd appreciate it!

@dvdchr
Copy link
Contributor Author

dvdchr commented Oct 8, 2021

@mattmiklic Yeah, I've already used the semantic .error color, which translates to Red 50 in the code. Based on the screenshot I've provided, the RGB values are indeed different when compared with the Color Studio (TIL on the site, so thanks for the reference 🙂 ). Perhaps the screenshot caused the colors to shift a little bit? I've checked using a Digital Color Meter, and some of the RGB values are shifted by 1-2 values:

Screenshot Color Studio
Screen Shot 2021-10-09 at 00 15 17 Screen Shot 2021-10-09 at 00 16 18

In Dark Mode, we should lighten the red by a shade to make the red text more legible on a dark background, so that should be Red 40.

Noted 👍 . I'll update the color for the dark theme soon. Thanks for the input, sir!


Update: Addressed in d9ccadc.

Copy link
Contributor

@ScoutHarris ScoutHarris left a comment

Choose a reason for hiding this comment

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

Looks good!

:shipit:

@ScoutHarris
Copy link
Contributor

Hey @dvdchr . I need this + develop to hook up the Delete Permanently button, so I took the liberty of merging this PR. Hope that's ok. 😬

@ScoutHarris ScoutHarris merged commit 9ee85bb into develop Oct 8, 2021
@ScoutHarris ScoutHarris deleted the issue/17087-delete-button branch October 8, 2021 22:41
@mattmiklic
Copy link
Member

Perhaps the screenshot caused the colors to shift a little bit?

Yep that's probably it. Looks good, thanks for updating the dark mode color as well!

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

Successfully merging this pull request may close these issues.

3 participants