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

Stabilizing follow-us plugin #2160

Merged
merged 18 commits into from
Feb 14, 2024

Conversation

jmealing
Copy link
Contributor

@jmealing jmealing commented May 11, 2023

  • Remove provisional classes
  • Adjustment to margins in lists to have better aligned spacing and touch target areas
  • Remove margin on h2 that is causing it to have a large gap on mobile

To do:

@jmealing jmealing marked this pull request as draft May 11, 2023 17:42
@jmealing jmealing temporarily deployed to github-ci May 11, 2023 17:42 — with GitHub Actions Inactive
@jmealing
Copy link
Contributor Author

Unfortunately I can't get the local preview working on my devices, but I coded the page elsewhere to validate these edits should work to improve the design spacing

@duboisp duboisp changed the title Update _base.scss Stabilizing follow-us plugin May 15, 2023
@duboisp
Copy link
Member

duboisp commented May 15, 2023

@Garneauma when you have time this week, can you build a site demo + link to where to download the compiled version in .zip.

@jmealing jmealing temporarily deployed to github-ci August 25, 2023 16:02 — with GitHub Actions Inactive
@duboisp duboisp added the Query: Project item Part of a github project label Nov 3, 2023
@Garneauma Garneauma self-assigned this Nov 28, 2023
@Garneauma
Copy link
Contributor

@jmealing Unfortunately, some of the changes you made are breaking the older pattern of the follow us plugin (.followus). Is the intent to stop supporting the older pattern?

Secondly, I think this stabilization will require us to create a documentation for the follow us plugin in order for us to test and track upcoming changes. Are you interested in creating this documentation or are you relying on us to create it? If you are relying on us, we will most likely create a separate PR that would include your few change suggestions regarding margins.

Finally, could you send the link to the guidance for this plugin?

Thank you!

@Garneauma Garneauma assigned jmealing and unassigned Garneauma Dec 1, 2023
@jmealing
Copy link
Contributor Author

jmealing commented Dec 5, 2023

@Garneauma The intention is to stop supporting the older pattern and replace it with this one

I'll discuss the other items with the DTO team and get back to you

The guidance is in staging here: https://deploy-preview-308--design-system-canada-ca.netlify.app/common-design-patterns/social-media-channels

@jmealing
Copy link
Contributor Author

jmealing commented Dec 5, 2023

@Garneauma Could you clarify what changes would be breaking between the new pattern and the old? They use two entirely different classes gc-followus vs followus

@Garneauma
Copy link
Contributor

@Garneauma Could you clarify what changes would be breaking between the new pattern and the old? They use two entirely different classes gc-followus vs followus

They indeed use a different class. However, they use the same pseudo-class to build the styles in SASS (%social-media-icons-followus-logo-properties). Since you removed "provisional" from the pseudo-class name at line 71, it now overwrites the pseudo-class defined at line 5 (which now has the exact same name). I have suggested changes to fix the issue.

@jmealing
Copy link
Contributor Author

jmealing commented Dec 7, 2023

@Garneauma Thanks for clarifying, I see what happened now. I accepted your changes

@Garneauma
Copy link
Contributor

@jmealing The old pattern looks good now. Thank you.

I was comparing the output of GCWeb with your latest changes vs the guidance and noticed the spacing is a little different for the stacked version and very different for the inline version. Is this intentional?

GCWeb stacked:
Screen Shot 2023-12-07 at 12 56 10 PM

Guidance stacked:

GCWeb inline:
Screen Shot 2023-12-07 at 12 56 34 PM

Guidance inline:

@jmealing
Copy link
Contributor Author

jmealing commented Dec 7, 2023

@Garneauma You're right, there shouldn't be that much spacing between the heading and the icons for inline. I am not able to preview my edits reliably which makes it a challenge. It's about more than double what it should be.

The guidance draft image does need to be updated too. The slightly increased spacing between the individual icons themselves is correct, and a bit more space between header and icons than that pictured is intended.

Copy link
Contributor

@Garneauma Garneauma left a comment

Choose a reason for hiding this comment

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

@jmealing The following change should fix the issue.

@jmealing
Copy link
Contributor Author

jmealing commented Dec 8, 2023

@Garneauma Thank you! Changes accepted

@Garneauma
Copy link
Contributor

Garneauma commented Dec 20, 2023

@jmealing This component will need a documentation page and working examples (for stacked and inline).

Are you willing to try the new data-first approach for the documentation? If so, you can start with the sites/date-modified files and edit it for gc-follow-us. You will only need the 2 HTML files and the JSON-LD file. Don't bother with the "changesets" and "iterations" properties in the JSON-LD as they can be a bit tricky. I can suggest the content for those once you are done.

If not, we can discuss another approach. Thanks!

@Garneauma Garneauma assigned jmealing and unassigned Garneauma Dec 20, 2023
@jmealing
Copy link
Contributor Author

@Garneauma guidance and examples have been created in the sites folder. Please let me know if we need additional changes. I flagged some items to send to translation

@jmealing jmealing removed their assignment Jan 15, 2024
@Garneauma Garneauma force-pushed the feature/gc-follow-us-stable branch from b2e1ba1 to c74843d Compare February 6, 2024 21:21
@Garneauma Garneauma assigned duboisp and unassigned jmealing Feb 7, 2024
@duboisp
Copy link
Member

duboisp commented Feb 12, 2024

Pre-approved upon review and testing

duboisp
duboisp previously approved these changes Feb 14, 2024
Copy link
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

Reviewed and tested, everything work as expected.

The following will need to be completed, we will do it after this is merged considering this is only documentation related

  • Documents the to be deprecated asset "twitter.svg"
  • Document the new asset "x.svg"

@duboisp duboisp dismissed Garneauma’s stale review February 14, 2024 14:44

Changes was reviewed and addressed by Garneauma

@duboisp duboisp merged commit 39e7ea1 into wet-boew:master Feb 14, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Query: Project item Part of a github project
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

3 participants