[UI] Move import button to the side of the Install one#5339
[UI] Move import button to the side of the Install one#5339flavioislima wants to merge 9 commits intomainfrom
Conversation
| height: auto; | ||
| grid-template-columns: 1fr; | ||
| grid-template-rows: min-content 1fr 1fr min-content; | ||
| grid-template-rows: min-content 400px 2fr min-content; |
There was a problem hiding this comment.
One thing I noticed with this change (well I guess it's not this PR but the previous one) is that the Import game option works differently if you use it from the details page and if you use in with the install modal open: when you use the import game without the install dialog the game is added with the default/shared wine prefix configure, but if you use the Import game button in the install dialog it sets the correct per-game prefix.
I wonder if this button should actually add a new modal that lets you configure the wine stuff and pick a folder instead of just a file picker?
Not sure if that needs to be fixed in this PR, but it makes this button way more visible now so maybe it's good to address that here so we don't forget to fix it later?
Another things that I noticed (but maybe it's been like this forever) is that while the game is being imported we don't disable the install/import buttons, so a user can trigger both the import and after that open the download dialog and trigger an installation or another import. We should disable the Install dialog when importing too
| } | ||
| } | ||
|
|
||
| @media screen and (max-width: 1000px) { |
…uncher into ui/import_btn_change
|
@arielj implemented a simple modal to select the wine prefix and version now:
Also disabled the button while importing. |
arielj
left a comment
There was a problem hiding this comment.
One thing I noticed with these changes is that now there's no way to import a game from the library view because the install dialog only shows the install button now
I think this can be also handled in relation to the other comment I made about maybe not changing the DownloadDialog but adding a new ImportDialog, then the Import Game button in the DownloadDialog could close itself and open the ImportDialog, so it will work also from the library
one more thing that I'm not sure if it's correct or not is that the global isImporting state is well global but at the same time related to importing a game and also it's not clear for example why we wouldn't be able to import a GOG game wile an Epic game is being imported
maybe instead of making it a global state it can just check the state of that particular game?
| <> | ||
| <span> | ||
| {`${t('install.disk-space-left', 'Space Available')}: `} | ||
| {!isImportMode && ( |
There was a problem hiding this comment.
there are lines inside this checking if isImportMode is true/false but this first statement already ensures isImportMode is false anywhere inside it
| useContext(ContextProvider) | ||
|
|
||
| const { action = 'install' } = useInstallGameModal() | ||
| const isImportMode = action === 'import' |
There was a problem hiding this comment.
I don't think it's a good idea to reuse the install dialog for the import dialog
this is doing a lot of unnecessary stuff when isImportMode: is fecthing install info, SDLs info, calculating the download size, checking available disk space, checking DLC, and then the return JSX is full of {!isImportMode && ... for everything except the children
I think it will be way more clean to have an ImportDialog based on what we have inthe DownloadDialog but without all the extra stuff we are ignoring
| ) : null} | ||
| </SideloadDialog> | ||
| )} | ||
| {renderContent()} |
There was a problem hiding this comment.
is there a reason to move this out of here into a function? I ask because it's not clear to me from the diff is something was changed or just extracted into a function (and not sure why either, what's the benefit if this?)
| wineCrossoverBottle | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
what happens if the import fails here? I understand here we are saving the setting before importing the game?
I think it's better to send the settings in the window.api.importGame call and create the settings on the backend once we know the game was imported successfully
| } | ||
| } | ||
|
|
||
| @container (max-width: 470px) { |
There was a problem hiding this comment.
this can be problematic with translations
we can do this to not depend on a pixel size here:
// in line 313
.installButtons {
gap: var(--space-xs);
justify-content: flex-start; // line 280 is making this centered which doesn't work
flex-wrap: wrap; // so it wraps as needed
}but we also need to remove the max-width of .gameConfigContainer .mainInfoWrapper .mainInfo .buttons, now it's max-width: 250px; but there's no reason for that, there's more space already




Use the following Checklist if you have changed something on the Backend or Frontend: