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

Fix New Sandbox Modal #1194

Merged
merged 4 commits into from
Oct 23, 2018
Merged

Fix New Sandbox Modal #1194

merged 4 commits into from
Oct 23, 2018

Conversation

SaraVieira
Copy link
Contributor

What kind of change does this PR introduce?
Fixes the add create sandbox modal

What is the current behavior?
Overflowing because of server templates

What is the new behavior?
screenshot 2018-10-16 at 15 29 15

Checklist:

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table

@SaraVieira SaraVieira requested a review from CompuIves October 16, 2018 13:30
@SaraVieira
Copy link
Contributor Author

screenshot 2018-10-16 at 15 34 36
why ? :(

@CompuIves
Copy link
Member

This is great! We need to merge some other stuff in to make it 'official'.

These ones:

Then I have to add them to the server too, but that is almost no work. I was also thinking that we can enable Sapper now (it's just adding the showOnHomepage: true for Sapper!

@SaraVieira
Copy link
Contributor Author

Sapper updated 🎉

Merge this when the importers are merged as well then :)

@@ -93,12 +92,27 @@ export default class Modal extends React.PureComponent {
))}

<Title>Server Templates</Title>
{mainServerRows.map((ts, i) => (
// eslint-disable-next-line react/no-array-index-key
Copy link

@franklinjavier franklinjavier Oct 18, 2018

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

The items rendered will not be changing, so nothing bad should come of using the index as the key here.

Choose a reason for hiding this comment

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

Yeah, I know. But if someone isolate and compose this part of code, may affect it.
I always do this to ensure it will work regardless of context

@CompuIves CompuIves merged commit 0429f52 into codesandbox:master Oct 23, 2018
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