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

Final QA + fixes #843

Merged
merged 4 commits into from
Nov 11, 2024
Merged

Final QA + fixes #843

merged 4 commits into from
Nov 11, 2024

Conversation

bryceerobertson
Copy link
Member

I did a lot of troubleshooting in various places so chances are there's some unnecessary/ugly code in the mix

package.json Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

@mruwnik can confirm but I believe you don't want to have the package.json included in your commit. I'm not sure how it gets updated (if ever) though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it depends. It's good to update things, as long as you check that nothing broke after the version bumps... In general it's usually better to split it into 2 PRs

Copy link
Collaborator

@Aprillion Aprillion Nov 10, 2024

Choose a reason for hiding this comment

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

separating changes of all files that were need while updating package.json (into different PR or multiple commits in the same PR) are nicer for reviewers when other files needed to be updated, so see only changes due to lib version update in 1 place vs. changes independent from that in 2nd pace ... but it doesn't matter for final code, only whether it works => "nice to have" for future PRs, IMHO no need to change this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok thanks, noted for the future. I'll just cross my fingers that it didn't break anything this time

Copy link
Collaborator

Choose a reason for hiding this comment

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

if it works locally, it shoud be fine 🤞 but just in case, I deployed your branch to https://stampy-ui2.aprillion.workers.dev/ if you want to double check 😅 (most often, library updates tend to be in the category "either all works or all broken" and the deploy looks good to me on homepage and a few random pages, so as I said, it should be fine 🍀)

app/components/HowCanIHelp/HelpItem.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

not related to this PR in particular, but I'm surprised so much content was added into source code and not parsed from a google doc / coda ... does it mean the i18n efforts were de-prioritized?

Copy link
Member Author

Choose a reason for hiding this comment

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

app/components/LinkCard/linkcard.css Outdated Show resolved Hide resolved
app/routes/how-can-i-help.community.tsx Show resolved Hide resolved
<div className="flexbox padding-bottom-56">
<h2 className="teal-500 padding-bottom-4 col-8" id="new-to-aisafety">
If you're somewhat new to AI safety, we recommend an introductory overview
If you{'\u2019'}re somewhat new to AI safety, we recommend an introductory overview
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we use the right single quotation mark consistently for content from gdocs as well as from source code JSX for all the contractions, please? I personally prefer &apos;->' for source-code readability, but if ’ comes from gdocs, then &rsquo;=={'\u2019'}->’ is probably better indeed

Copy link
Member Author

Choose a reason for hiding this comment

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

@LeMurphant would know more but from a quick look at the site it appears there isn't too much consistency:

image

I personally think the ’ looks prettier than the ', and FWIW that what's the Figma has. I would advocate making that consistent across the site (only) if it's relatively easy

@melissasamworth melissasamworth merged commit 6e13565 into stampy-redesign Nov 11, 2024
1 check passed
@melissasamworth melissasamworth deleted the bryce-qa-2 branch November 11, 2024 22:36
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.

5 participants