#2581 - Improve login-error prompt style #2586
Conversation
| } | ||
|
|
||
| const result = await sbp('gi.ui/prompt', promptOptions) | ||
| if (!result) { | ||
| if (result) { |
There was a problem hiding this comment.
Now that 'Yes' is the primary button, this check must be negated.
| // Sometimes, the query string for modal has both 'modal' and 'subcontent' eg.) '/app/?modal=SignupModal&subcontent=Prompt' | ||
| if (to.query.subcontent) { | ||
| const subContentTo = to.query.subcontent.split('+').pop() | ||
| if (subContentTo !== this.activeSubContent) { | ||
| // Try to find the target subcontent in the list of subcontent | ||
| const i = this.subcontent.indexOf(subContentTo) | ||
| if (i !== -1) { | ||
| this.subcontent = this.subcontent.slice(0, i) | ||
| } else { | ||
| this.subcontent = [subContentTo] | ||
| } | ||
| } |
There was a problem hiding this comment.
This is irrelevant to the issue itself but a fix for a bug I encountered while working on the issue.
The bug was that, when the app is reloaded and it has 'sign-up' modal and also 'login-error' prompt on top of it, this.subcontent becomes a weird string value (it was alphabet t in my case) while theModal.vue is assuming that this.subcontent is an array of strings at all times.
As a result, I was unable to close SignupModal.vue. So had to update this part with fixes like L:71 above.
group-income
|
||||||||||||||||||||||||||||
| Project |
group-income
|
| Branch Review |
sebin/task/#2581-improve-login-error-prompt
|
| Run status |
|
| Run duration | 11m 09s |
| Commit |
|
| Committer | Sebin Song |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
10
|
Skipped
|
0
|
Passing
|
112
|
| View all changes introduced in this branch ↗︎ | |
SebinSong
changed the title
There was a problem hiding this comment.
Great work @SebinSong!
I could merge as-is, since everything works, or we could add the 'filled' styled as part of this PR, or leave that as a separate issue. Let me know which you prefer!
| secondaryButton: L('Yes') | ||
| primaryButton: L('Yes'), | ||
| secondaryButton: L('No'), | ||
| primaryButtonStyle: 'primary', // make primary button 'filled' style |
There was a problem hiding this comment.
Why is it that the default style is called 'outlined', and to make it filled, we call it 'primary' instead of 'filled'?
Wouldn't it make more sense to call it 'filled'?
Note: if you change this, you'd need to look through the rest of the codebase for 'primary' and make sure everything is updated accordingly. Or you could add the style as an alias to primary, without changing the existing definition of primary, which would be safer.
There was a problem hiding this comment.
Why is it that the default style is called 'outlined', and to make it filled, we call it 'primary' instead of 'filled'?
Well, the Prompt.vue had both buttons as 'outlined' style before this PR and I'm only changing the style of one button for a particular prompt(Login Error) in this PR..
Wouldn't it make more sense to call it 'filled'?
Nowhere in this project's CSS is below button style called 'filled' but instead it is called 'primary'.
(The term 'filled' is something people at my other company use for that kind of button style. I think I got it from there)
closes #2581
[ screenshot ]