[MM-61717] Refresh Settings Modal without Bootstrap #3337
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
devinbinnie
added
2: Dev Review
devinbinnie
requested review from
abhijit-singh,
a team and
enahum
and removed request for
a team
|
@devinbinnie in the description you ask for us to test on the builds generated by the PR but they seem to be failing |
|
@enahum Looks like it went through - https://github.com/mattermost/desktop/actions/runs/13389165374 |
devinbinnie
force-pushed
the
MM-61717
branch
from
303fe4e to
19a9238
Compare
devinbinnie
added
Build Apps for PR
|
Here are the test results below: Test Summary for Linux on commit ed79ac3New failed tests found on Linux:
Test Summary for macOS on commit ed79ac3All stable tests passed on macOS. Test Summary for Windows on commit ed79ac3New failed tests found on Windows:
|
github-actions
bot
removed
the
Run Desktop E2E Tests
devinbinnie
added
the
Run Desktop E2E Tests
|
@abhijit-singh @enahum @hmhealey This should be ready for review at your convenience. |
|
Here are the test results below: Test Summary for Linux on commit f156901New failed tests found on Linux:
Test Summary for macOS on commit f156901All stable tests passed on macOS. Test Summary for Windows on commit f156901New failed tests found on Windows:
|
github-actions
bot
removed
the
Run Desktop E2E Tests
abhijit-singh
added
Build Apps for PR
enahum
approved these changes
Feb 25, 2025
| @@ -38,16 +38,17 @@ function handleShowOnboardingScreens(showWelcomeScreen: boolean, showNewServerMo | |||
| log.debug('handleShowOnboardingScreens', {showWelcomeScreen, showNewServerModal, mainWindowIsVisible}); | |||
|
|
|||
| if (showWelcomeScreen) { | |||
| if (ModalManager.isModalDisplayed()) { | |||
| const welcomeScreen = ModalManager.modalQueue.find((modal) => modal.key === 'welcomeScreen'); | |||
There was a problem hiding this comment.
not major, but it would be good to have welcomeScreen as a constant
There was a problem hiding this comment.
Agreed, we should actually do this across the board for all of the modal names.
Seems like something AI could help me hash out real quick ;)
I'll put it in a follow-up change though.
There was a problem hiding this comment.
| const welcomeScreen = ModalManager.modalQueue.find((modal) => modal.key === 'welcomeScreen'); | ||
| if (welcomeScreen?.view.webContents.isLoading()) { | ||
| welcomeScreen?.view.webContents.once('did-finish-load', () => { | ||
| const welcomeScreenTest = ModalManager.modalQueue.find((modal) => modal.key === 'welcomeScreen'); |
There was a problem hiding this comment.
and then use it here as well
| <button | ||
| id={`RadioSetting_${id}_${option.value}`} | ||
| className='RadioSetting__radio' | ||
| key={index} |
There was a problem hiding this comment.
this is better as index.toString() or `${index}`
| <div className='ServerSetting__serverList'> | ||
| {(servers.map((server, index) => ( | ||
| <div | ||
| key={index} |
abhijit-singh
requested changes
Feb 25, 2025
There was a problem hiding this comment.
Thanks @devinbinnie! Looks good. I spotted a few issues:
- Pressing
Escto close the settings modal crashes the app with this error:
TypeError: Cannot read properties of undefined (reading 'stack')
at Object.createErrorReport (/private/var/folders/0c/6xhd1slj4tv6mw_bcfdt94s40000gn/T/AppTranslocation/D9C204DE-C58D-472D-8881-928B80864B47/d/Mattermost.app/Contents/Resources/app.asar/index.js:2:118270)
at Object.showExceptionDialog (/private/var/folders/0c/6xhd1slj4tv6mw_bcfdt94s40000gn/T/AppTranslocation/D9C204DE-C58D-472D-8881-928B80864B47/d/Mattermost.app/Contents/Resources/app.asar/index.js:2:116774)
at process.<anonymous> (/private/var/folders/0c/6xhd1slj4tv6mw_bcfdt94s40000gn/T/AppTranslocation/D9C204DE-C58D-472D-8881-928B80864B47/d/Mattermost.app/Contents/Resources/app.asar/index.js:2:116568)
at process.emit (node:events:518:28)
at emitUnhandledRejection (node:internal/process/promises:250:13)
at warnWithErrorCodeUnhandledRejectionsMode (node:internal/process/promises:404:19)
at processPromiseRejections (node:internal/process/promises:470:17)
at process.processTicksAndRejections (node:internal/process/task_queues:96:32)
- We should probably give a minimum height to the modal. How we have done it in other places (Team settings modal, user settings modal) is by having a min-height: 475px; applied to the modal body—we can add the same here.
- For the
Add server,Edit servermodals, we need to have another overlay, or push the original settings modal below the overlay.
- In the dark mode, some of the borders and dividers aren't visible. Can we check if they're using the
center-channel-color-rgbvariable with a 0.08 opacity?
yasserfaraazkhan
requested changes
Feb 25, 2025
There was a problem hiding this comment.
@devinbinnie if we delete all servers, we still see the modal open and click through the different options.
This was using
I don't think that's a problem, the user may want to look at other settings while in the modal, so closing it on them would be jarring. @abhijit-singh If you're okay with this I'll leave it as-is. |
It'll be great to add some empty state text, possibly along with an illustration, instead of just blank space after the heading - Figma design Also, I just noticed one more small thing — Clicking just below the modal in some cases (when the modal height is less) does not close it. Ideally clicking anywhere outside the modal should close it. |
|
Also, one last thing — can we call this something like 
|
I'll go with Desktop App Settings to be more specific. |
Can't reproduce this, can you try again after I put a new build up? I might have inadvertently fixed it. |
devinbinnie
added
Build Apps for PR
|
Thanks @devinbinnie for the quick updates! Just a couple things remaining:
2. The border colour issue seems to be still there. Somehow, the divider in the `General` tab between `Download location` and the other settings seems to be behaving fine. Can we match the divider between the modal header and body, and also the dividers on the `Server` tab to match the colours of that divider in `General`?
|
yasserfaraazkhan
approved these changes
Feb 26, 2025
|
@abhijit-singh Fixed! Issue with the CSS variables
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduced a completely redesigned Settings Modal, that completely abandons the original Bootstrap-based design (and Bootstrap itself) and introduces a look and feel similar to the rest of our Mattermost apps. Credit to @abhijit-singh for the work on these designs :)
The page is broken down into a number of individual
Settingcomponents that represent individual controls that can be re-used and re-purposed for different settings. All of the settings themselves are then defined indefinition.tsxwhich references these components. A few exceptions needed to be made where either additional data was required, special designs were need, or some dependencies needed to be satisfied. However, the hope is that adding/modifying/removing settings should be pretty easy from now on.I would highly recommend downloading one of the available PR builds for this, and trying it out on different OSes to make sure that all of the settings are working and that no unexpected UI/UX bugs show up. I have done some preliminary testing on each OS to ensure nothing glaring exists, but to make this bulletproof it will take some time and a fine tooth comb.
Ticket Link
https://mattermost.atlassian.net/browse/MM-61717
Device Information
This PR was tested on:
Screenshots
Release Note