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

feat(admin-panel): Add information pertaining to bounceType #12877

Merged
merged 1 commit into from
May 17, 2022

Conversation

xlisachan
Copy link
Contributor

@xlisachan xlisachan commented May 13, 2022

Because:

  • it would provide more information when clearing email bounces

This commit:

  • adds descriptions for bounce type/subtype based on Amazon SES
  • adds Undetermined to BounceType, and OnAccountSuppressedList to BounceSubType
  • updates/renames ResultListItem to ResultTableRow
  • fixes the verified/not verified style as they were not appearing green/red
  • updates Connected Services as tables as well
  • PR adding descriptions to documentation in Ecosystem Platform

Closes #12155

Questions:

  • dedent is not applying multi-line strings o_O - removed dedent
  • Missing BounceType? - Undetermined - added to BounceType
  • Missing BounceSubType? - OnAccountSuppressionList- added to BounceSubType

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

BEFORE
Screen Shot 2022-05-13 at 2 50 51 PM

AFTER
Screen Shot 2022-05-16 at 4 46 22 PM

Connected Services
Screen Shot 2022-05-13 at 2 53 28 PM

@xlisachan xlisachan requested a review from a team as a code owner May 13, 2022 19:05
@xlisachan xlisachan marked this pull request as draft May 13, 2022 19:16
@xlisachan
Copy link
Contributor Author

We use dedent in the Swagger docs to help with multi-line strings and markdowns, but it doesn't seem like they're being applied. I tried using ts-dedent since these are .ts files, but that doesn't seem to work either. Any ideas on what could be happening?

  • There is supposed to be a new line before and after "⚠️ Important"

Screen Shot 2022-05-13 at 2 10 45 PM

  • The text is not a link

Screen Shot 2022-05-13 at 2 08 41 PM

@xlisachan xlisachan force-pushed the adminpanel_bounceType branch 3 times, most recently from 580aedd to 337de9b Compare May 13, 2022 19:59
@xlisachan xlisachan changed the title feat(admin-panel): Add information pertaining to bounceType - WIP feat(admin-panel): Add information pertaining to bounceType May 13, 2022
@xlisachan xlisachan marked this pull request as ready for review May 13, 2022 20:52
@xlisachan xlisachan force-pushed the adminpanel_bounceType branch from 337de9b to 9c8dce5 Compare May 14, 2022 02:33
@dschom
Copy link
Contributor

dschom commented May 16, 2022

@xlisachan The dedent library just smartly strips leading white space on newlines, but it doesn't actually add any html, which is why you aren’t seeing any effect. There are two ways I can think of dealing with this. One approach could be using wrapping the text with pre block and applying the appropriate styling. Another option, that I think would be better, is to return jsx with html tags to render the output as desired from the bounce-descritions.tsx.

Copy link
Contributor

@dschom dschom left a comment

Choose a reason for hiding this comment

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

Aside from the dedent issue, which isn't that big a deal imo, this looks good to me.

@xlisachan xlisachan force-pushed the adminpanel_bounceType branch from 765d443 to cc4206e Compare May 16, 2022 20:49
@xlisachan
Copy link
Contributor Author

Thanks for the feedback, @dschom ! I have made the following updates:

  • Removed dedent/ts-dedent as it was not working as expected
  • Updated the descriptions into an array and used .map() to inject each string into a p tag instead, per @LZoog's suggestion
  • Updated the first screenshot within the PR issue
  • Updated the description file from a .tsx to .ts file, per @LZoog's suggestion
  • Added Undetermined to BounceType, and OnAccountSuppressedList to BounceSubType

Can you please review this PR again when you have a moment? Thank you!

@xlisachan
Copy link
Contributor Author

Noting that Dan reviewed the revisions and gave the OK :)
Screen Shot 2022-05-17 at 11 59 29 AM

Because:

* it would provide more information when clearing email bounces

This commit:

* adds descriptions for bounce type/subtype based on Amazon SES

Closes #12155
@xlisachan xlisachan force-pushed the adminpanel_bounceType branch from cc4206e to b465f92 Compare May 17, 2022 16:54
@xlisachan xlisachan merged commit 458110a into main May 17, 2022
@xlisachan xlisachan deleted the adminpanel_bounceType branch May 17, 2022 17: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.

[ecosystem platform] Add information pertaining to bounceType
2 participants