Details
Tested on Chrome/Safari and works as expected. However, since localhost/comm is faster than prod, the change can be seen less quickly in the videos. However, this diff is very similar to D3605 which works fine on prod.
Diff Detail
- Repository
- rCOMM Comm
- Branch
- master
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| web/modals/threads/settings/thread-settings-general-tab.css | ||
|---|---|---|
| 32 | This is the current height of the Save button (technically it's 46.5px). This rule just ensures that when the content of the button changes (from 'Save' to the loading indicator) the button doesn't shrink or change its dimensions. | |
| web/modals/threads/settings/thread-settings-general-tab.react.js | ||
| 143–148 | ||
I'm generally not a good person to put as a first pass reviewer. Take a look at the list of scenarios you should add me in here. Of course there's some flexibility here (eg. if I request something, or if the issue is very high-pri and you need somebody to take a look on the weekend or something) but basically there should be a good reason
(It can be helpful to explain why you're adding me to a review if it's not obvious)
(It can be helpful to explain why you're adding me to a review if it's not obvious)
Whoops! I think I accidentally added you as a reviewer for this diff. Thanks for resigning, your reasoning makes sense.
Looks great! Good thinking to re-use the LoadingIndicator we had from the LoginForm component.
I wonder if we want to close the settings dialog once the settings are saved? I think I'd prefer for it to be left open so I could make further changes if wanted.
Might be a better experience for the loading spinner to transition into a green button with a check mark and a message like "Settings saved." or something like that?
I wonder if we want to close the settings dialog once the settings are saved? I think I'd prefer for it to be left open so I could make further changes if wanted.
Might be a better experience for the loading spinner to transition into a green button with a check mark and a message like "Settings saved." or something like that?
I agree with both of these assessments. Going to land this change (just the LoadingIndicator) and work on those other two changes as separate diffs themselves to increase modularity and make it easier to review.