refactor: [M3-8775] - Make the Support Ticket UI look slightly less broken #11144
Conversation
bnussman-akamai
requested review from
carrillo-erik and
cpathipa
and removed request for
a team
bnussman-akamai
changed the title
|
Coverage Report: ✅ |
There was a problem hiding this comment.
Improvements look great - I completely missed the Formatting Tips dropdown in #10979, so thank you for the follow up. Can we also track this in an internal ticket by copying the PR description over and getting this PR title updated?
Left a few comments after looking through this in light and dark mode. The one real minor regression is with the top padding in Preview mode.
packages/manager/src/features/Support/SupportTicketDetail/TabbedReply/PreviewReply.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Support/SupportTickets/SupportTicketDialog.tsx
Outdated
Show resolved
Hide resolved
| <Typography> | ||
| You can use Markdown to format your{' '} | ||
| {props.isReply ? 'reply' : 'question'}. For more examples see this{' '} | ||
| <Link external to="https://commonmark.org/help/"> | ||
| Markdown cheatsheet | ||
| </Link> | ||
| </Typography> | ||
| <Typography className={classes.header}> | ||
| <strong>Examples</strong> | ||
| <Typography fontFamily={(theme) => theme.font.bold} variant="h3"> |
There was a problem hiding this comment.
I don't think we want Formatting Tips and this Examples header to both be h3s, semantically.
There was a problem hiding this comment.
I would have used h4 if it was properly implemented 😭
Any recommendations on what to do here?
We could implement h4 in the theme relatively easily. I will just use bold text for now in abb4f5d
There was a problem hiding this comment.
h4 is ginormous, ha. I see now that we've only got h1-h3 and subtitle1 in our theme. It doesn't look like subtitle1 is what we want here, either. My preference would be to implement h4 in theme, since that's really the way to avoid running into this again in the future. Seems like it would be a small lift?
There was a problem hiding this comment.
Just saw the latest edit to your comment which I think you updated at almost the same time I posted mine. Agreed that bolded looks fine here.
There was a problem hiding this comment.
The changes look good and it improves the previous iteration. In the future, if you run into something similar and want to explore another workaround consider the following.
The <Typography> component can actually take a component prop and styling can be accomplished with the variant and sx props like this:
<Typography
component="span"
sx={(theme) => padding: theme.spacing(1)}
variant="body1"
>
packages/manager/src/features/Support/SupportTicketDetail/TabbedReply/MarkdownReference.tsx
Outdated
Show resolved
Hide resolved
bnussman-akamai
changed the title
bnussman-akamai
changed the title
|
Changes look good. Just found one small regression with the top padding for the Formatting Tips Accordion. The padding is removed when the Accordion is collapsed but it's added back when the Accordion is expanded.
|
bnussman-akamai
added
Approved
Cloud Manager E2E
|
||||||||||||||||||||||||||||||||||||||||
| Project |
Cloud Manager E2E
|
| Branch Review |
develop
|
| Run status |
|
| Run duration | 28m 24s |
| Commit |
|
| Committer | Banks Nussman |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
1
|
Pending
|
2
|
Skipped
|
0
|
Passing
|
444
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/e2e/core/linodes/clone-linode.spec.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| clone linode > can clone a Linode from Linode details page |
Screenshots
Video
|
|
cypress/e2e/core/account/service-transfer.spec.ts • 1 flaky test
| Test | Artifacts | |
|---|---|---|
| Account service transfers > can initiate and cancel a service transfer |
Screenshots
Video
|
|
Description 📝
Preview 📷
How to test 🧪
As an Author I have considered 🤔