feature:check json value #4519

AbnerHuang2 · 2022-08-14T02:41:07Z

What's the purpose of this PR

feature:check json value

Which issue(s) this PR fixes:

Fixes #4142

Brief changelog

to be more specific, I made a short video for it, It works like that.

2022-08-13.3.24.52.mov

Follow this checklist to help us incorporate your contribution quickly and easily:

Read the Contributing Guide before making this pull request.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit tests to verify the code.
Run mvn clean test to make sure this pull request doesn't break anything.
Update the CHANGES log.

github-actions · 2022-08-14T02:41:25Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

AbnerHuang2 · 2022-08-14T02:41:46Z

I have read the CLA Document and I hereby sign the CLA

nobodyiam · 2022-08-14T13:50:36Z

This feature looks great!
However, I'm wondering is it possible to display the json check button only for potential json values? As I could imagine there will be many check in the future, e.g. yaml check, XML check, etc. The edit modal will look very complicated if there are a lot of checks.

codecov-commenter · 2022-08-14T13:51:48Z

Codecov Report

Merging #4519 (848fa90) into master (0b6c4e9) will increase coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #4519      +/-   ##
============================================
+ Coverage     53.41%   53.43%   +0.02%     
  Complexity     2708     2708              
============================================
  Files           492      492              
  Lines         15390    15390              
  Branches       1596     1596              
============================================
+ Hits           8220     8224       +4     
+ Misses         6611     6607       -4     
  Partials        559      559

Impacted Files	Coverage Δ
...ervice/service/ReleaseMessageServiceWithCache.java	`85.88% <0.00%> (-1.18%)`	⬇️
...ework/apollo/internals/RemoteConfigRepository.java	`88.34% <0.00%> (+3.06%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

AbnerHuang2 · 2022-08-14T15:27:30Z

This feature looks great! However, I'm wondering is it possible to display the json check button only for potential json values? As I could imagine there will be many check in the future, e.g. yaml check, XML check, etc. The edit modal will look very complicated if there are a lot of checks.

yeah, you are right, that will be terrible if there were so many check button. but it is hard to show the check button if the value is not in JSON format, unless the value is in [] or {}, then I can show the check button. What do you think?

nobodyiam · 2022-08-15T13:08:50Z

unless the value is in [] or {}, then I can show the check button.

I think we could first try this.

AbnerHuang2 · 2022-08-15T15:07:40Z

I think we could first try this.

ok, I'll do it tomorrow.

AbnerHuang2 · 2022-08-16T03:07:51Z

@nobodyiam I have done this feature, it works like that.

2022-08-16.11.04.42.mov

nobodyiam · 2022-08-19T00:47:18Z

apollo-portal/src/main/resources/static/views/component/item-modal.html

@@ -46,15 +46,22 @@ <h4 class="modal-title">
                    <label class="col-sm-2 control-label">{{'Component.ConfigItem.ItemValue' | translate }}</label>
                    <div class="col-sm-10" valdr-form-group>
                        <textarea id="valueEditor" name="value" class="form-control" rows="6" tabindex="2"
-                            ng-required="false" ng-model="item.value">
+                            ng-required="false" ng-model="item.value" ng-blur="check()">


I think the experience of the blur event is not quite good, as it only displays the check json button after the focus leaves the textarea. However, that normally is when user clicks the submit button, so that the user would not even see the check json button.
Is it possible to display the check json button when the user is editing the value? e.g. to check whether the value is json text after user stops typing for several seconds?
Another potential area to improve is to display the check json button when the user just clicks the edit button but not editing anything in the textarea.

BTW, would you please also update the CHANGES.md?

BTW, would you please also update the CHANGES.md?

I'll update the CHANGES.md later.

This feature looks great! However, I'm wondering is it possible to display the json check button only for potential json values? As I could imagine there will be many check in the future, e.g. yaml check, XML check, etc. The edit modal will look very complicated if there are a lot of checks.

from the begining, I thought you might want to show the check json button while the value is JSON , so I set the check rule is the value in {} or [] . but when user is typing , the value is not in {} or [] . because they type { or ] when they finish type.
if then ,

Is it possible to display the check json button when the user is editing the value? e.g. to check whether the value is json text after user stops typing for several seconds?

the way you mentioned is the same as blur. and there is also comment to write after value. but in most time, after writing the value, user may click submit button directly, maybe onchange is better than blur.
since the cost of check is tiny, I'll replace blur with onchange . maybe it is better for UE(User Experience).

BTW,

Another potential area to improve is to display the check json button when the user just clicks the edit button but not editing anything in the textarea.

as the rule (show check button when the value in {} or []) set before , the check button will not show in this scenario.

If you want show check button when user is typing , we will face the situation you metioned before , that is lots of check in defferent format. I think it may not an good idea.

since the cost of check is tiny, I'll replace blur with onchange

I'm not sure whether the onchange event could meet this requirement as it is triggered after the input loses focus?

If you want show check button when user is typing , we will face the situation you metioned before , that is lots of check in defferent format. I think it may not an good idea.

I think the main issue that causes the problem is the item doesn't have format info. Maybe we could consider to add format info to the item entity, see #1517 (comment).

In this case, I suppose we need to store the format in the Item table? Besides, we also need to take a look at the Text edit mode to see whether it is compatible or not.

ok, maybe it will be more complicated in this way. what's a touch issue! it has taken too much time in this issue. maybe we should give the matter further thought. in the long term, if you want to support the item value format check more precise, store the format maybe is a good choice. but it should take more time to think about.
at present, shall we just show check button in onchange event to satisfy json format check?

or can I add your wechat , if I have any idea, I can talk to you. and it may more effective for communicate. if we reach a consensus, I can post and pr to deal with it. what do you think? if you don't wanna to be bothered, we can also talk in the comment.

Sounds good. You can find my email from my profile page and tell me your contact info.

well, nice. I'll send you later. and I'll review and test my code and commit tonight.

AbnerHuang2 · 2022-08-28T06:27:51Z

I have update CHANGES.md and add json check when modal shown. @nobodyiam

nobodyiam · 2022-08-28T07:53:16Z

CHANGES.md

@@ -5,6 +5,7 @@ Release Notes.
 Apollo 2.1.0

 ------------------
+* [Add a potential json value check feature](https://github.com/apolloconfig/apollo/pull/4519)


Please add this to the bottom.

nobodyiam

LGTM

nobodyiam reviewed Aug 19, 2022

View reviewed changes

AbnerHuang2 mentioned this pull request Aug 26, 2022

openapi请求如果key是a.bS/TT/AT，则报404错误 #4447

Closed

AbnerHuang2 force-pushed the master branch 2 times, most recently from d89d20d to efcd57d Compare August 28, 2022 07:47

nobodyiam reviewed Aug 28, 2022

View reviewed changes

check json value

848fa90

AbnerHuang2 force-pushed the master branch from efcd57d to 848fa90 Compare August 28, 2022 08:12

nobodyiam approved these changes Aug 28, 2022

View reviewed changes

nobodyiam merged commit 150fe19 into apolloconfig:master Aug 28, 2022

github-actions bot locked and limited conversation to collaborators Aug 28, 2022

nobodyiam added this to the 2.1.0 milestone Feb 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature:check json value #4519

feature:check json value #4519

AbnerHuang2 commented Aug 14, 2022

github-actions bot commented Aug 14, 2022 •

edited

Loading

AbnerHuang2 commented Aug 14, 2022

nobodyiam commented Aug 14, 2022

codecov-commenter commented Aug 14, 2022 •

edited

Loading

AbnerHuang2 commented Aug 14, 2022

nobodyiam commented Aug 15, 2022

AbnerHuang2 commented Aug 15, 2022

AbnerHuang2 commented Aug 16, 2022

nobodyiam Aug 19, 2022

nobodyiam Aug 19, 2022

AbnerHuang2 Aug 22, 2022

AbnerHuang2 Aug 22, 2022

nobodyiam Aug 26, 2022

nobodyiam Aug 27, 2022

AbnerHuang2 Aug 27, 2022

AbnerHuang2 Aug 27, 2022

nobodyiam Aug 27, 2022

AbnerHuang2 Aug 27, 2022

AbnerHuang2 commented Aug 28, 2022

nobodyiam Aug 28, 2022

AbnerHuang2 Aug 28, 2022

nobodyiam left a comment

feature:check json value #4519

feature:check json value #4519

Conversation

AbnerHuang2 commented Aug 14, 2022

What's the purpose of this PR

Which issue(s) this PR fixes:

Brief changelog

github-actions bot commented Aug 14, 2022 • edited Loading

AbnerHuang2 commented Aug 14, 2022

nobodyiam commented Aug 14, 2022

codecov-commenter commented Aug 14, 2022 • edited Loading

Codecov Report

AbnerHuang2 commented Aug 14, 2022

nobodyiam commented Aug 15, 2022

AbnerHuang2 commented Aug 15, 2022

AbnerHuang2 commented Aug 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbnerHuang2 commented Aug 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nobodyiam left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 14, 2022 •

edited

Loading

codecov-commenter commented Aug 14, 2022 •

edited

Loading