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

style: APP-432 certificate footer update #2523

Merged
merged 4 commits into from
Nov 18, 2024
Merged

Conversation

r41ph
Copy link
Contributor

@r41ph r41ph commented Oct 30, 2024

Description

https://regennetwork.atlassian.net/browse/APP-432

Author Checklist

I have...

  • provided a link to the relevant issue or specification
  • provided instructions on how to test
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

How to test

  1. https://deploy-preview-2523--regen-marketplace.netlify.app/certificate/WyJyZXRpcmVtZW50cyIsMSwxNzQzNDg1NiwwLDBd?name=
  2. Check the X icon and the 'Print certificate' button in the certificate share section look like in the designs.

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items
.

I have...

  • confirmed all author checklist items have been addressed
  • reviewed code correctness and readability
  • verified React components follow DRY principles
  • reviewed documentation is accurate
  • reviewed tests
  • manually tested (if applicable)

Copy link

netlify bot commented Oct 30, 2024

Deploy Preview for regen-website ready!

Name Link
🔨 Latest commit 53890b5
🔍 Latest deploy log https://app.netlify.com/sites/regen-website/deploys/673b28551d7a720008ed43ed
😎 Deploy Preview https://deploy-preview-2523--regen-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@r41ph r41ph requested a review from blushi October 30, 2024 14:11
@r41ph
Copy link
Contributor Author

r41ph commented Oct 30, 2024

@erikalogie see testing instructions

On top of the issues mentioned in the JIRA ticket, I noticed alignment issues when testing the responsiveness of the share section (see video) and have made some updates to improve that too. Could you have a look please?

Screen.Recording.2024-10-30.at.14.03.24.mov

@erikalogie
Copy link
Collaborator

This looks so much better! Thanks @r41ph!

Comment on lines 78 to 82
sx={{
[theme.breakpoints.up('md')]: {
gap: theme.spacing(4),
},
}}
Copy link
Member

Choose a reason for hiding this comment

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

can we use tailwind instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was more convenient to do it this way because some Tailwind classes don't work with preflight disabled. However, I have now implemented those classes - see update in the tailwind.css file.

Copy link
Member

Choose a reason for hiding this comment

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

I see
FYI spacing prop can also take responsive values, eg spacing={{ xs: 1, sm: 2, md: 3 }} but I guess it's probably best to work with tailwind anyway

@r41ph r41ph force-pushed the fix-APP-432-style-issues branch from b82891c to a327ae7 Compare November 6, 2024 13:55
@r41ph r41ph requested a review from blushi November 6, 2024 14:03
@@ -75,26 +76,28 @@ function CertificatePage(): JSX.Element {
{certificateData && (
<Grid
container
className={classes.share}
alignItems="flex-end"
sx={{ displayPrint: 'none' }}
Copy link
Member

Choose a reason for hiding this comment

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

just noticing this got removed, I'll add it back before merging

Copy link

netlify bot commented Nov 18, 2024

Deploy Preview for terrasos ready!

Name Link
🔨 Latest commit 53890b5
🔍 Latest deploy log https://app.netlify.com/sites/terrasos/deploys/673b28554af06c0008c0d874
😎 Deploy Preview https://deploy-preview-2523--terrasos.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@blushi blushi force-pushed the fix-APP-432-style-issues branch from 8021be4 to 53890b5 Compare November 18, 2024 11:43
@blushi blushi merged commit 47a2207 into dev Nov 18, 2024
18 checks passed
@blushi blushi deleted the fix-APP-432-style-issues branch November 18, 2024 11:52
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.

3 participants