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

QA and various fixes #832

Merged
merged 4 commits into from
Nov 7, 2024
Merged

QA and various fixes #832

merged 4 commits into from
Nov 7, 2024

Conversation

bryceerobertson
Copy link
Member

This stuff from #817:
Screenshot 2024-11-05 at 23 15 25

Some stuff too complicated for me remains, e.g. making the small cards at the bottom display in a 2x2 grid when there are four of them (instead of three). I'll collect them all up and make issues.

@bryceerobertson
Copy link
Member Author

@mruwnik I'm afraid the lint ghosts continue to haunt me. You're going to need to teach me how to fix it myself, lest I bother you every time to fix it

@LeMurphant
Copy link
Collaborator

LeMurphant commented Nov 6, 2024

@bryceerobertson here are the steps for debugging

  • Run the following command to see if the lint will fail: npx tsc && npm run prettier && npm run eslint
  • In this case, there were a few things that the lint did not like. I fixed it by running this: npx prettier --write app/{components,routes}
  • This automatically fixed the lint issues. I checked the git diff to make sure the changes made sense, they did
  • Ran the lint command again (the first one), gave me a few more errors. I had to diagnose them manually, but they made sense
stampy-ui/app/routes/how-can-i-help.career.tsx
  235:29  error  Irregular whitespace not allowed  no-irregular-whitespace
  235:70  error  Irregular whitespace not allowed  no-irregular-whitespace
  237:69  error  Irregular whitespace not allowed  no-irregular-whitespace
  238:45  error  Irregular whitespace not allowed  no-irregular-whitespace

stampy-ui/app/routes/how-can-i-help.volunteer.tsx
  8:8  error  'LonsImg' is defined but never used  @typescript-eslint/no-unused-vars
  8:8  error  'LonsImg' is defined but never used  unused-imports/no-unused-imports

The numbers at the start are the line number and character number. I fixed all the errors, and can push the fix to resolve the issues, but I can also let you diagnose them for yourself if you want

@mruwnik
Copy link
Collaborator

mruwnik commented Nov 6, 2024

npm run lint:fix should also solve as much as it can, giving you the places to fix

@bryceerobertson
Copy link
Member Author

Managed it! Thank you 🙏🏼

Copy link
Contributor

@melissasamworth melissasamworth left a comment

Choose a reason for hiding this comment

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

This is amazing 🙏, I usually don't like people touching my stuff but you actually made it better. Thank you!

@@ -21,15 +21,15 @@ const helpItems = [
{
title: 'Donate',
description:
'The AI safety field is constrained by funding—financial help is critical at this moment',
'The AI safety field is constrained by funding—financial help is critical at the moment',
Copy link
Contributor

Choose a reason for hiding this comment

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

I had liked this, but can't explain why. So you get your way.

@@ -55,12 +55,13 @@ const titles = {
}

const knowledgeDescriptions = {
donate: 'The more you know about this topic, the further your donation efforts can go',
donate:
'Knowing more about this topic will allow you to make wiser donation decisions, meaning your funds will have a greater impact',
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I like this actually.

@@ -79,7 +79,7 @@ input {

.page {
margin: auto;
max-width: 1608px; /* multiple of 12 for .col-x, 1 col at max width = 134px */
max-width: 1440px;
Copy link
Contributor

Choose a reason for hiding this comment

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

I've made the grid (widths) static, so I think this line is just obsolete. But that's fine

@@ -117,6 +117,13 @@ h2 {

/* typography classes */

.extra-large {
Copy link
Contributor

Choose a reason for hiding this comment

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

image Ah, I see what you were going for. This was my mistake. I don't want to add another extra-large class, but rather make all the large classes consistent. I think I will come in and fix this soon so I don't get confused when this system doesn't match my Figma later. For now it works, sorry for the confusion.

@@ -99,7 +104,7 @@ const Online = () => (
className="padding-bottom-40"
>
<p className="grey default">
You can volunteering for projects and attend protests. People are divided regarding whether
You can volunteer for projects and attend protests. People are divided regarding whether
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this copy just because what we're trying to illustrate here something about how communities are grouped, not whether attending protests is important. If we really don't like including the word protests, I would just choose another activity that is common to illustrate our point. This is the only piece of feedback so far I am bullish on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed

@melissasamworth melissasamworth merged commit afe43d2 into stampy-redesign Nov 7, 2024
1 check passed
@melissasamworth melissasamworth deleted the bryce-QA-1 branch November 7, 2024 01:18
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.

4 participants