-
Notifications
You must be signed in to change notification settings - Fork 878
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
Rework Game Creation dialog #7179
base: master
Are you sure you want to change the base?
Conversation
b08b39d
to
b60a944
Compare
b60a944
to
c6601ae
Compare
e4f7841
to
18f78fe
Compare
First impressions:
|
} | ||
|
||
.emptyProject:focus { | ||
background-color: var(--theme-hover-background-color) |
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.
Feels weird to have a hover color that is not linked to a status (warning, success) or a component (surface dark or light)
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.
Could you add a transition on the background-color also?
i18n={i18n} | ||
columnsCount={getItemsColumns(windowSize, isLandscape)} | ||
onlyShowGames | ||
rowToInsert={{ |
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
<Trans> | ||
Create an account to store your project online. | ||
</Trans> | ||
{!isOnHomePage && isSelectedGameTemplateOwned && ( |
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 feel like isSelectedGameTemplateOwned
means something else, like isExampleOrOwnedGameTemplateOpened
.
I was troubled by the check on selectedExampleShortHeader
below and I was already surprised to see that isSelectedGameTemplateOwned
is true if there is no game template selected
import { ExampleStoreContext } from '../AssetStore/ExampleStore/ExampleStoreContext'; | ||
import { type ExampleShortHeader } from '../Utils/GDevelopServices/Example'; | ||
import { useResponsiveWindowSize } from '../UI/Responsive/ResponsiveWindowMeasurer'; | ||
import { GridList, GridListTile } from '@material-ui/core'; |
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.
👮♀️
return ( | ||
<GridListTile style={style}> | ||
<div | ||
className={classNames({ |
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.
you don't need the classNames util here, you can just have className={classes.container}
<Column> | ||
{isMobile && <Spacer />} | ||
<Line justifyContent="flex-start" noMargin> | ||
{/* Add a hidden text to match the height of the other tiles on the row */} |
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.
the grid should normally handle that, is it really needed?
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.
Maybe a height: 100% is needed somewhere
const generateProject = React.useCallback( | ||
async () => { | ||
if (disabled) return; | ||
if (!profile) return; |
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.
if
s can be merges
{!isCompatible && ( | ||
<AlertMessage kind="error"> | ||
<Trans> | ||
Unfortunately, this example requires a newer version of GDevelop to |
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.
Should this message be a bit more positive: "Download the latest version to check out this example!"
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.
Excellent idea.
import AuthenticatedUserContext from '../../Profile/AuthenticatedUserContext'; | ||
import { PrivateGameTemplateStoreContext } from '../PrivateGameTemplates/PrivateGameTemplateStoreContext'; | ||
import { GridList } from '@material-ui/core'; |
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.
👮♀️
/** | ||
* This method allocates examples and private game templates between the | ||
* build section carousel and grid. | ||
* `numberOfItemsExclusivelyInCarousel` controls the number of items that | ||
* should appear in the carousel only. The rest appears in both the carousel | ||
* and the grid. | ||
*/ | ||
export const getExampleAndTemplateItemsForBuildSection = ({ | ||
export const getExampleAndTemplateTiles = ({ |
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.
We have created a monster 😅
Lots of changes including: