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

Improve Quick Customization flow #6978

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ClementPasteau
Copy link
Collaborator

@ClementPasteau ClementPasteau commented Sep 19, 2024

Mainly simplify the flow, with fewer objects, fewer behavior props to modify, a title to update and a simple save/publish end screen.

Todo:

  • fix a bug where trying to swap an asset after having navigated through the store on another page creates an error. -> should reset the store

@@ -202,9 +201,8 @@ const PageBreakNavigation = ({
}}
disabled={pageBreakIndex <= 0}
/>
<RaisedButton
<FlatButton
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that button was too visible in the shop, where it's only to navigate pages. I think it's better this way

</IconButton>
)}
<Column expand useFullHeight noMargin>
<SearchBar
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

only show searchbar when swapping asset

@@ -792,7 +798,8 @@ export const AssetStore = React.forwardRef<Props, AssetStoreInterface>(
hideGameTemplates={hideGameTemplates}
hideDetails={!!assetSwappedObject}
/>
) : openedAssetShortHeader ? (
) : // Do not show the asset details if we're swapping an asset.
openedAssetShortHeader && !assetSwappedObject ? (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that logic is both for asset swapping in the objects list & asset swapping in the quick customization.
I'm wondering if we should differentiate both.
-> quick custom, keep it simple
-> objects list, give all the search options, including seeing the details page

if (assetsListInterface) {
assetsListInterface.scrollToPosition(0);
assetsListInterface.setPageBreakIndex(0);
value={searchText}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we should probably initiate the searchText to something similar to the object being swapped, to get better results.
Though, some names may not give results at all

<QuickCustomizationPropertiesVisibilityDialog
open={!!selectedQuickCustomizationPropertiesBehavior}
onClose={() => setSelectedQuickCustomizationPropertiesBehavior(null)}
propertyNames={selectedQuickCustomizationPropertiesBehavior
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for some reason, passing the gdMapStringPropertyDescriptor to that dialog was not working properly. when getting the keys in the dialog, the result was not the one excepted. I couldn't figure out what was wrong & ended up passing the names directly from there.

@@ -229,8 +228,6 @@ const styles = {
},
container: { flex: 1, minWidth: 0 },
separator: {
marginRight: -marginsSize,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this was causing a nasty horizontal scroll

@@ -118,7 +118,7 @@ export const ExtensionOptionsEditor = ({
return (
<I18n>
{({ i18n }: { i18n: I18nType }) => (
<ColumnStackLayout noMargin>
<ColumnStackLayout noMargin expand>
Copy link
Collaborator Author

@ClementPasteau ClementPasteau Sep 19, 2024

Choose a reason for hiding this comment

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

for some reason, the fields in that dialog were not full width, but it doesn't seem to have changed recently so maybe we just never noticed?

@@ -177,10 +179,12 @@ const OnlineGameLink = ({
() => {
if (exportStep === 'done') {
setTimeBeforeExportFinished(timeForExport); // reset.
setIsShareDialogOpen(true);
if (shouldShowShareDialog) {
setIsShareDialogOpen(true);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

allow not opening the dialog when in quick customization and let the quickPublish component display the link themselves

getStorageProviderOperations(CloudStorageProvider);
saveProjectAsWithStorageProvider({
requestedStorageProvider: CloudStorageProvider,
forcedSavedAsLocation: {
Copy link
Collaborator Author

@ClementPasteau ClementPasteau Sep 19, 2024

Choose a reason for hiding this comment

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

I went for a new prop here to force the name of the project and skip the "Select the Cloud Project name" dialog. There is no unicity on the name of a cloud project, so I think we're fine

// Automatically save project to the cloud if no provider is set.
const storageProvider = getStorageProvider();
console.log('storageProvider', storageProvider);
if (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should we always save in cloud and not look at the current storage provider?

await this._fetchUserProfileWithoutThrowingErrors();
await this._fetchUserProfileWithoutThrowingErrors({
// When creating an account, avoid showing the email verification dialog right away.
dontNotifyAboutEmailVerification: true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is fine, because most users use a SignUpProvider now + when opening the Profile Dialog that popup will then show (or next time the app is launched)

return;
}

propertiesQuickCustomizationVisibilities.set(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I went for a set prop by prop, maybe we'd rather:
store the values in state & have a "Save" button in the Dialog that clears the Map & sets again all the props.
This would avoid possible values in the future not being removed if they change name

@@ -135,7 +135,7 @@ type AccordionProps = {|
*/
export const Accordion = React.forwardRef<AccordionProps, MUIAccordion>(
(props, ref) => {
const { costlyBody, ...otherProps } = props;
const { costlyBody, noMargin, ...otherProps } = props;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

react complaining that this props was passed to the MUIComponent which didn't support it

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.

1 participant