-
Notifications
You must be signed in to change notification settings - Fork 3
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
The specification list is now more responsive #15
Conversation
Was causing errors on Macs
Hopefully this will fix the problem this time
src/view/basic/SpecificationItem.tsx
Outdated
<List className={classes.bordered}> | ||
{specification.sdks.map(sdk => ( | ||
<ListItem> | ||
<SdkItem sdk={sdk} key={sdk.id} /> |
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.
key
should be on ListItem
.
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.
yes :) ty
src/view/basic/SpecificationItem.tsx
Outdated
}, | ||
sdkHeaderActions: { | ||
flexGrow: 0, | ||
flexShrink: 1, |
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.
Kinda minor, but flex-grow
is 0 by default and flex-shrink
is 1 by default. Could we make it more concise by removing default values?
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'm a fan of being explicit but I suppose it forces the JS file payload to be bigger.
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.
Can you remove the min-width
ones at least? Don't see the need for those at all.
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.
Read the thing i said below, flexbox children have min-width: auto
src/view/basic/SpecificationItem.tsx
Outdated
flexGrow: 0 | ||
}, | ||
summaryDescription: { | ||
minWidth: 0, |
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.
Same with min-width
, 0 is the default anyway.
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.
Actually on flexbox children min-width is set to auto
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.
Oh ok, fair enough
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.
Fix the key thing and I'll approve :)
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.
Okay donzies 🎉
src/view/basic/SpecificationItem.tsx
Outdated
minWidth: 0, | ||
flexBasis: '220px', | ||
flexShrink: 1, | ||
flexGrow: 0 |
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.
Did you want to remove these 2 as well? (since you removed the other one, don't have to, just though I'd check)
Reworked a little bit of the GUI so it's more responsive. Here's a comparison.
Changed (max width of the list is 800px):
Original: