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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8dff6c5
fix modal
SaraVieira Oct 16, 2018
7fbcf3f
lint :(
SaraVieira Oct 16, 2018
fbd729a
Update sapper.js
SaraVieira Oct 16, 2018
7e5ade7
Merge branch 'master' into fix-modal
SaraVieira Oct 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 26 additions & 12 deletions packages/app/src/app/pages/Dashboard/Content/CreateNewSandbox/Modal/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ const usedTemplates = sortBy(

const TEMPLATE_BASE_WIDTH = 150;
const MAIN_TEMPLATE_BASE_WIDTH = 190;
const SERVER_TEMPLATE_BASE_WIDTH = 150; // 150

export default class Modal extends React.PureComponent {
selectTemplate = template => {
Expand All @@ -39,21 +38,21 @@ export default class Modal extends React.PureComponent {
const paddedWidth = width;
const mainTemplates = usedTemplates.filter(t => t.main && !t.isServer);
const otherTemplates = usedTemplates.filter(t => !t.main && !t.isServer);
const serverTemplates = usedTemplates.filter(t => t.isServer);
const mainServerTemplates = usedTemplates.filter(t => t.main && t.isServer);
const otherServerTemplates = usedTemplates.filter(
t => !t.main && t.isServer
);

const mainTemplatesPerRow = Math.max(
1,
paddedWidth / MAIN_TEMPLATE_BASE_WIDTH
);
const templatesPerRow = Math.max(1, paddedWidth / TEMPLATE_BASE_WIDTH);
const serverTemplatesPerRow = Math.max(
1,
paddedWidth / SERVER_TEMPLATE_BASE_WIDTH
);

const mainRows = chunk(mainTemplates, mainTemplatesPerRow);
const rows = chunk(otherTemplates, templatesPerRow);
const serverRows = chunk(serverTemplates, serverTemplatesPerRow);
const mainServerRows = chunk(mainServerTemplates, mainTemplatesPerRow);
const serverRows = chunk(otherServerTemplates, templatesPerRow);

return (
<Container
Expand All @@ -64,7 +63,7 @@ export default class Modal extends React.PureComponent {
<InnerContainer forking={forking} closing={closing}>
<Title>Client Templates</Title>
{mainRows.map((ts, i) => (
// eslint-disable-next-line
// eslint-disable-next-line react/no-array-index-key
<Templates key={i}>
{ts.map(t => (
<Template
Expand All @@ -78,7 +77,7 @@ export default class Modal extends React.PureComponent {
))}

{rows.map((ts, i) => (
// eslint-disable-next-line
// eslint-disable-next-line react/no-array-index-key
<Templates style={{ fontSize: '.8rem' }} key={i}>
{ts.map(t => (
<Template
Expand All @@ -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 •
edited
Loading

Choose a reason for hiding this comment

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

You should not use indexes for keys. This can negatively impact performance and may cause issues with state.

refs
https://dev.to/pureooze/a-simple-list-render-optimization-for-react--5715
https://reactjs.org/docs/lists-and-keys.html#keys
facebook/react#1342 (comment)
https://medium.com/@robinpokorny/index-as-a-key-is-an-anti-pattern-e0349aece318

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.

Copy link

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

<Templates key={i}>
{ts.map(t => (
<Template
width={Math.floor(paddedWidth / mainTemplatesPerRow - 16)}
key={t.name}
template={t}
selectTemplate={this.selectTemplate}
/>
))}
</Templates>
))}

{serverRows.map((ts, i) => (
// eslint-disable-next-line
<Templates style={{ fontSize: '.9rem' }} key={i}>
// eslint-disable-next-line react/no-array-index-key
<Templates style={{ fontSize: '.8rem' }} key={i}>
{ts.map(t => (
<Template
width={Math.floor(paddedWidth / serverTemplatesPerRow - 16)}
small
width={Math.floor(paddedWidth / templatesPerRow - 16)}
key={t.name}
template={t}
selectTemplate={this.selectTemplate}
Expand Down
1 change: 1 addition & 0 deletions packages/common/templates/gatsby.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,6 @@ export default new Template(
isServer: true,
mainFile: ['/src/pages/index.js'],
showOnHomePage: true,
main: true,
}
);
1 change: 1 addition & 0 deletions packages/common/templates/next.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,6 @@ export default new Template(
mainFile: ['/pages/index.js'],
backgroundColor: decorateSelector(() => '#000000'),
showOnHomePage: true,
main: true,
}
);
1 change: 1 addition & 0 deletions packages/common/templates/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export default new Template(
{
isServer: true,
showOnHomePage: true,
main: true,
mainFile: ['/pages/index.vue', '/pages/index.js', '/src/pages/index.js'],
}
);
1 change: 1 addition & 0 deletions packages/common/templates/nuxt.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,6 @@ export default new Template(
isServer: true,
mainFile: ['/pages/index.vue'],
showOnHomePage: true,
main: true,
}
);
2 changes: 1 addition & 1 deletion packages/common/templates/sapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,6 @@ export default new Template(
{
isServer: true,
mainFile: ['/src/routes/index.html'],
showOnHomePage: false,
showOnHomePage: true,
}
);
1 change: 0 additions & 1 deletion packages/homepage/content/docs/2-importing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ We infer sandbox settings based on several files in a repository.
| Template - React | If `package.json` dependencies contains `react-scripts`. |
| Template - React-Typescript | If `package.json` dependencies contains `react-scripts-ts`. |
| Template - Svelte | If `package.json` dependencies contains `svelte`. |
| Template - Node | If `package.json` dependencies contains `ember-cli`. |

Additionally, you may specify a `template` property in your `./sandbox.config.json` file.

Expand Down