-
Notifications
You must be signed in to change notification settings - Fork 10
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
enhancement/issue 176 add bluesky link to social tray component #179
base: main
Are you sure you want to change the base?
enhancement/issue 176 add bluesky link to social tray component #179
Conversation
✅ Deploy Preview for super-tapioca-5987ce ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Left some comments and some additional call outs here:
- Could we please fill out the PR template with a link to the issue and summary of changes?
- Having seen CI run, we'll need to update the test case for this component to account for a fourth icon (see the
SOCIAL_ICONS
array in the test file, and validate by runningnpm run test
)
Let me know if you have any questions! 👍
@@ -18,6 +19,13 @@ export default class SocialTray extends HTMLElement { | |||
${discordIcon} | |||
</a> | |||
</li> | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the formatting here looks off, not sure if you had a chance to run prettier before submitting? You can run npm run format
to automatically format, otherwise CI will fail when run here.
edit: hmm, looks like it didn't fail, that's interesting. Well, if npm run format
doesn't fix it, could we try and make the changes here match the existing formatting conventions of other social tray icon links? Thanks!
|
||
<li class="${styles.socialIcon}"> | ||
<a href="https://bsky.app/profile/projectevergreen.bsky.social" title="Bluesky" | ||
style="display: inline-block; transform: scale(1.7) translateY(-2px);"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on some quick testing, it didn't seem like the display: inline-block
was needed at least.
Otherwise, is there anyway we can get the rest of this styling back into the SVG at all? I didn't have to do it for other SVG icons in the social tray, so hoping we don't have to do a one-off styles here ideally.
If it is needed, I would probably suggest we just add a specific CSS class for this in social-tray.module.css
Ok lemme give it another shot with these instructions in mind
…On Thu, 6 Feb 2025, 20:46 Owen Buckley, ***@***.***> wrote:
***@***.**** commented on this pull request.
Thanks!
Left some comments and some additional call outs here:
1. Could we please fill out the PR template with a link to the issue
and summary of changes?
2. Having seen CI run
<https://github.com/ProjectEvergreen/www.greenwoodjs.dev/actions/runs/13178624056/job/36803802208>,
we'll need to update the test case for this component to account for a
fourth icon (see the SOCIAL_ICONS array in the test file, and validate
by running npm run test)
Let me know if you have any questions! 👍
------------------------------
In src/components/social-tray/social-tray.js
<#179 (comment)>
:
> @@ -18,6 +19,13 @@ export default class SocialTray extends HTMLElement {
${discordIcon}
</a>
</li>
+
the formatting here looks off, not sure if you had a chance to run
prettier before submitting? You can run npm run format to automatically
format, otherwise CI will fail when run here.
edit: hmm, looks like it didn't fail, that's interesting. Well, if npm
run format doesn't fix it, could we try and make the changes here match
the existing formatting conventions of other social tray icon links? Thanks!
------------------------------
In src/components/social-tray/social-tray.js
<#179 (comment)>
:
> @@ -18,6 +19,13 @@ export default class SocialTray extends HTMLElement {
${discordIcon}
</a>
</li>
+
+ <li class="${styles.socialIcon}">
+ <a href="https://bsky.app/profile/projectevergreen.bsky.social" title="Bluesky"
+ style="display: inline-block; transform: scale(1.7) translateY(-2px);">
based on some quick testing, it didn't seem like the display: inline-block
was needed at least.
Screenshot.2025-02-06.at.12.39.11.PM.png (view on web)
<https://github.com/user-attachments/assets/bea312a9-df59-4da5-a09e-9f1260b67530>
Otherwise, is there anyway we can get the rest of this styling back into
the SVG at all? I didn't have to do it for other SVG icons in the social
tray, so hoping we don't have to do a one-off styles here ideally.
If it is needed, I would probably suggest we just add a specific CSS class
for this in *social-tray.module.css*
—
Reply to this email directly, view it on GitHub
<#179 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOZBHURFSYZOZPSYE5XWZHL2OONYDAVCNFSM6AAAAABWTM2VKCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKOJZGQZTMMZUGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***
com>
|
Related Issue
Summary of Changes