#548 P0: Chris/fix previous picks #549
Conversation
|
Gridiron Survivor Application
Project name: Gridiron Survivor Application
Only deployments on the production branch are activated automatically. If you'd like to activate this deployment, navigate to your deployments. Learn more about Appwrite Function deployments.
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| /> | ||
| ) : ( | ||
| <span | ||
| className="text-xs h-16 w-16 text-primary pt-6 text-center" |
There was a problem hiding this comment.
I'm worried about the a11y of the color and the small text size, did you check this?
There was a problem hiding this comment.
@ryandotfurrer I did not check this. hmmmmm @shashilo what do you think?
There was a problem hiding this comment.
Fine for now. A11Y audit can come after.
| /> | ||
| ) : ( | ||
| <span | ||
| className="text-xs h-10 w-full text-primary pt-2" |
There was a problem hiding this comment.
I'm worried about the a11y of the color and the small text size, did you check this?
There was a problem hiding this comment.
@ryandotfurrer I did not check this. hmmmmm @shashilo what do you think?
There was a problem hiding this comment.
Fine for now. A11Y audit can come after.
| () => { | ||
| expect(screen.getByTestId('weekly-picks')).toBeInTheDocument(); | ||
| }, | ||
| { timeout: 5000 }, |
There was a problem hiding this comment.
Question: is this an arbitrary number for the timeout or is there a reason for using 5000?
There was a problem hiding this comment.
@jennifertieu great comment! I am not sure! LOL .... I removed it. I think initially I had a pause in something somewhere that caused the test to have to wait to see it in the DOM.
✅ removed and tests still work.
| { timeout: 5000 }, | ||
| ); | ||
|
|
||
| expect(screen.getByTestId('user-pick-history')).toBeInTheDocument(); |
There was a problem hiding this comment.
nit: I know there was a conversation on your other PR about declaring variable vs inline but I think this scenario where you have to write screen.getByTestId('user-pick-history') twice is a good use case to save it in a variable instead to reuse.
Both approaches work though! The code still runs but might save you or someone else in the future sometime. Of course, we'll see what Shashi thinks because I'm curious which is better or how much it matters😅.
There was a problem hiding this comment.
When there's more than 2, that's always a good case to make a variable.
There was a problem hiding this comment.
@shashilo @jennifertieu thanks! yes, moving forward I will be doing this when there are more than 2.
| expect(userPickHistoryLogos).toHaveLength(1); | ||
| expect(userPickHistoryLogos[0]).toHaveAttribute( | ||
| 'src', | ||
| '/_next/image?url=https%3A%2F%2Fexample.com%2Fbrowns.png&w=128&q=75', |
There was a problem hiding this comment.
Question: Why is the URL string encoded like this instead of url=https://example.com/browns.png? (same questions with the other URLs)
There was a problem hiding this comment.
@jennifertieu great question! the component is using Next Image. This is how Next Image formats the url for images so it can apply its customizations.
There was a problem hiding this comment.
@chris-nowicki replying late lol but that is good to know!! I'll keep that in mind when testing with Next in the future, thanks Chris!
fixes #548
SCREEN SHOTS