-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Refactor: Migrate ProductCard
to Chakra
#8685
Refactor: Migrate ProductCard
to Chakra
#8685
Conversation
@@ -156,7 +102,7 @@ export interface IProps { | |||
|
|||
const ProductCard: React.FC<IProps> = ({ |
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.
There is a TODO regarding adding loading state. What is the desired loading visual?
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.
Hmm not sure 😆 wouldn't worry too much since that comment is 20 months old and probably we are going to refactor this with the DS.
✅ ethereum-org-website-dev deploy preview ready
|
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.
Nice @TylerAPfledderer
One small thing to look at. The pills/badges are not aligned:
Img, | ||
Text, | ||
TextProps, | ||
} from "@chakra-ui/react" |
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.
import order
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.
@pettinarip curious to know if ESLint has ever been considered with a config that includes import/order
to enforce a specific order of the different import types?
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.
That would be a useful addition, yes. Hasn't been discussed/considered before AFAIK.
} | ||
} | ||
return ( | ||
<Box |
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.
These should be Badges in the upcoming refactor, right?
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.
@pettinarip Yes! I have made a note in #8601
@@ -156,7 +102,7 @@ export interface IProps { | |||
|
|||
const ProductCard: React.FC<IProps> = ({ |
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.
Hmm not sure 😆 wouldn't worry too much since that comment is 20 months old and probably we are going to refactor this with the DS.
Whoops, didn't see that! On it. |
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.
Found a bug on some pages using this component.
src/components/ProductCard.tsx
Outdated
{subjects && | ||
subjects.map((subject, idx) => ( | ||
<SubjectPill key={idx} subject={subject}> | ||
{subject} | ||
</SubjectPill> | ||
))} | ||
{hasRepoData && | ||
data.repository.languages.nodes.map( | ||
data.repository.language.nodes.map( |
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.
Wondering why this has changed 🤔
Currently I see an error breaking some pages. These two pages are loading but breaking after a second:
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.
Nicely done @TylerAPfledderer 🚀
Description
Migrate the
ProductCard
component to Chakra.Related Issue
#8632