-
Notifications
You must be signed in to change notification settings - Fork 542
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: improve user friendly on Upstream #1603
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1603 +/- ##
==========================================
- Coverage 71.46% 66.17% -5.29%
==========================================
Files 132 132
Lines 5375 5375
Branches 592 592
==========================================
- Hits 3841 3557 -284
- Misses 1290 1506 +216
- Partials 244 312 +68
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
rules={[ | ||
{ | ||
required: true, | ||
message: "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the message is an empty string? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because when users leave this field empty, the input box will have a red border to alert them 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems that when the required field is empty, the required message will appear behind the input box, so if we leave the message an empty string, there will be an empty line appear. anyway, since the host rewrite
become a disable option, this input box will never show in the page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
The new messages are much better. Thanks.
ping ~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LGTM |
The result of the discussion in the mailing list is that only one switch option |
`[DISCUSS] about the upstream request’s Host header It seems that the discussion is still in progress 🤔 why not review & take action on this improvement PR first? |
Please answer these questions before submitting a pull request
Why submit this pull request?
New feature or improvement
Improve i18n for Upstream's pass_host field & selector.