Skip to content
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

feat: show warning notification when dashboard version not matching apisix #1435

Merged
merged 4 commits into from
Feb 6, 2021

Conversation

LiteSun
Copy link
Member

@LiteSun LiteSun commented Feb 5, 2021

Please answer these questions before submitting a pull request

image

@LiteSun LiteSun requested a review from juzhiyuan February 5, 2021 08:46
Copy link
Member

@juzhiyuan juzhiyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notification will occur when the version gets mismatched and page refresh.

@juzhiyuan juzhiyuan requested review from imjoey and liuxiran February 5, 2021 09:05
@juzhiyuan
Copy link
Member

juzhiyuan commented Feb 5, 2021

Warning Notification is enough currently, we could support showing more information or recommendation if needed.

@imjoey
Copy link
Member

imjoey commented Feb 5, 2021

Warning Notification is enough currently, we could support showing more information or recommendation if needed.

+1 agreed. The recommendation of matched version is recommended. We could improve this later in other PR.

Copy link
Member

@imjoey imjoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Thanks @LiteSun .

@liuxiran
Copy link
Contributor

liuxiran commented Feb 5, 2021

fe e2e test failed, tried to solve in #1442

Copy link
Contributor

@liuxiran liuxiran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, it will very helpful for users, thanks

@liuxiran
Copy link
Contributor

liuxiran commented Feb 5, 2021

sorry I found that In addition to the test case run failed, there are some problems with lint, maybe it needs to solve in this pr

@@ -49,6 +51,9 @@ const GlobalHeaderRight: React.FC = () => {
if ((navTheme === 'dark' && layout === 'top') || layout === 'mix') {
className = `${styles.right} ${styles.dark}`;
}
useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don’t call Hooks inside loops, conditions, or nested functions. Instead, always use Hooks at the top level of your React function. You can follow the documentation here. (from https://stackoverflow.com/questions/57620799/react-hook-useeffect-is-called-conditionally)

just move this useEffect before line 44

@LiteSun
Copy link
Member Author

LiteSun commented Feb 6, 2021

sorry I found that In addition to the test case run failed, there are some problems with lint, maybe it needs to solve in this pr

Thanks for the review, I will fix it.

@codecov-io
Copy link

codecov-io commented Feb 6, 2021

Codecov Report

Merging #1435 (3c6f307) into master (9bd3a54) will decrease coverage by 11.04%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1435       +/-   ##
===========================================
- Coverage   68.08%   57.03%   -11.05%     
===========================================
  Files          48       48               
  Lines        3042     3042               
===========================================
- Hits         2071     1735      -336     
- Misses        728     1065      +337     
+ Partials      243      242        -1     
Impacted Files Coverage Δ
api/internal/core/entity/entity.go 18.75% <0.00%> (-81.25%) ⬇️
...l/handler/route_online_debug/route_online_debug.go 7.14% <0.00%> (-66.67%) ⬇️
api/internal/handler/upstream/upstream.go 47.45% <0.00%> (-35.60%) ⬇️
api/internal/handler/data_loader/route_import.go 30.24% <0.00%> (-34.68%) ⬇️
api/internal/log/log.go 30.00% <0.00%> (-30.00%) ⬇️
api/internal/utils/consts/api_error.go 25.00% <0.00%> (-25.00%) ⬇️
api/internal/core/store/storehub.go 50.00% <0.00%> (-24.49%) ⬇️
api/internal/utils/json_patch.go 34.48% <0.00%> (-24.14%) ⬇️
api/internal/filter/schema.go 31.93% <0.00%> (-23.53%) ⬇️
api/internal/handler/global_rule/global_rule.go 66.12% <0.00%> (-17.75%) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9bd3a54...3c6f307. Read the comment docs.

@juzhiyuan juzhiyuan merged commit 8796fa1 into apache:master Feb 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants