-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Home page information tiles #5271
Conversation
🦋 Changeset detectedLatest commit: 919ebe4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
9a31df0
to
3cd53ae
Compare
f200f07
to
48bfb5e
Compare
const DiscordIcon = () => <SVG src={discord} className={noShrink} />; | ||
const ExternalLinkIcon = () => <SVG src={externalLink} className={noShrink} />; | ||
|
||
export const getTilesData = ({ intl }: { intl: IntlShape }): InfoTile[] => [ |
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.
just a comment here - this force on you a specific structure for each tile, while you don't have eg. hundreds of them where you would need pre-defined structure. You have only few items and for that case simple composition would work better
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.
Imagine you need to change just a single tile now, in the way that not really fits this structure - it would need to probably opt-out from this
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.
I definitely understand the concern but in this case we had only tiles that had the exact same structure. Changing it to a more composable solution shouldn't take long if we decide that we're going to need it.
The base branch was changed.
48bfb5e
to
6a46add
Compare
d91af5a
to
919ebe4
Compare
What type of PR is this?
Related Issues or Documents
Usage Instructions, Screenshots, Recordings
Have you written tests?
[Optional] Description