Context: here
Improving user experience of ThreadSettingsModal
Main changes:
- keep submit buttons always on the bottom
- move error message above submit buttons
Details
Details
- Go to any chat
- Go to Thread Menu > Settings (ThreadSettingsModal)
- Switch between tabs
Now, all the submit buttons in form are keeping on the bottom of the modal.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Hm, at a high level is it the right abstraction to have errorMessage in SubmitButton but displayed "outside" of the button? Maybe we could borrow what we have on landing for the SubscriptionForm component and display the error within the button:
Maybe in a separate/subsequent diff?
Either way, accepting to unblock and adding @tomek as blocking
| web/modals/threads/settings/thread-settings-modal.css | ||
|---|---|---|
| 12 ↗ | (On Diff #17026) | Hm, can you provide some context on why we're using display: grid here instead of display: flex? |
Comment Actions
Hm, at a high level is it the right abstraction to have errorMessage in SubmitButton but displayed "outside" of the button? Maybe we could borrow what we have on landing for the SubscriptionForm component and display the error within the button:
Maybe in a separate/subsequent diff?
Either way, accepting to unblock and adding @tomek as blocking
It makes some sense, but this design has some flaws:
- When we display an error message instead of call to action, the purpose of the button might become confusing for the user, especially when retry is a possible action
- When validation fails there might be a couple of reasons and displaying all of them on the button doesn't make sense
These two aren't unavoidable, so with proper message and validation result it could be quite good.
The main reason I would like to avoid doing this here or as a followup is that it breaks consistency with other parts of the app. Also, we should focus on delivering business value instead of spending too much effort on minor improvements. So in this case, we can create a task, assign it a priority (probably low) and implement it when appropriate. Adding unrelated work to a scope of a task is one of the reasons we're not able to estimate things.
Hm, at a high level is it the right abstraction to have errorMessage in SubmitButton but displayed "outside" of the button?
We can call it SubmitSection I guess.
| web/modals/threads/settings/submit-button.css | ||
|---|---|---|
| 13 ↗ | (On Diff #17026) | We should prefer using line-height and padding / margin |
| web/modals/threads/settings/submit-button.react.js | ||
| 33–36 ↗ | (On Diff #17026) | When we always include a class, it is not necessary to write it inside object. Also, when a class is nullable, you can directly give it to classnames |
| 40 ↗ | (On Diff #17026) | Are these spaces necessary? |
| web/modals/threads/settings/thread-settings-modal.css | ||
| 5 ↗ | (On Diff #17026) | Does this change affect how existing components look like? |
Comment Actions
Adjust to code review:
- change SubmitButton name to SubmitSection
- remove min-height property
- simplify code and remove unnecessary spaces
| web/modals/threads/settings/thread-settings-modal.css | ||
|---|---|---|
| 5 ↗ | (On Diff #17026) | Yup, I tried to add some space for error message and to avoid resizing of modal when error message appears. Previously, error message was displayed below submit buttons and displaying it looks like: |
| 12 ↗ | (On Diff #17026) | Before this release, I tried a different approach with display: grid, but I tested it now and it seems that display: flex also works here, so I will change that. |
| web/modals/threads/settings/submit-section.css | ||
|---|---|---|
| 21 | What is the reason for using fractional height? | |
| web/modals/threads/settings/submit-section.react.js | ||
| 23 | Shouldn't we always have type: submit in submit section? | |
| 28–33 | You can take a similar approach when defining containerStyle | |
| web/modals/threads/settings/thread-settings-modal.css | ||
| 18 | This line wasn't changed in this diff, but we should use variables from typography | |
| 22 | I can see a lot of height: 100% in the code. Usually a more sensible approach is to use flex, e.g. flex: 1. Can we use it in more places? | |
Comment Actions
Adjust to code review:
- set button type as submit
- flex adjustments
- change height: 100% to flex: 1
- use classnames for containerStyle
- round fractional height
| web/modals/threads/settings/submit-section.css | ||
|---|---|---|
| 21 | I get this value from previous size of button, but I could adjust it to 45px  | |
| web/modals/threads/settings/thread-settings-modal.css | ||
| 12 ↗ | (On Diff #17026) | I tested it again and I had to adjust some additional props here, because display: flex destroying Relationship tab, but after this adjustment everything works fine. |
| web/modals/threads/settings/thread-settings-delete-tab.react.js | ||
|---|---|---|
| 26 ↗ | (On Diff #17170) | I think it might be better to have optional errorMessage. The reason is that mandatory error message means that we always have an error. We can also consider using ?string so that it's more convenient for SubmitSection. |
| web/modals/threads/settings/thread-settings-general-tab.react.js | ||
| 35 ↗ | (On Diff #17170) | It makes more sense to have SetState<?string> so that it's clear when there's no error. |