Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

Comments

fix: fix toggle not work in settings page#2476

Merged
a-b-r-o-w-n merged 12 commits intomasterfrom
wenyluo/fix
Apr 8, 2020
Merged

fix: fix toggle not work in settings page#2476
a-b-r-o-w-n merged 12 commits intomasterfrom
wenyluo/fix

Conversation

@VanyLaw
Copy link
Contributor

@VanyLaw VanyLaw commented Apr 2, 2020 •
edited
Loading

Description

remove the toggle feature in settings page

Task Item

close #2444

Screenshots

xhqme-6uewm

@github-actions
Copy link

github-actions bot commented Apr 2, 2020

Coverage Status

Coverage increased (+0.02%) to 39.684% when pulling 98c2489 on wenyluo/fix into e14850e on master.

a-b-r-o-w-n
a-b-r-o-w-n suggested changes Apr 2, 2020
View reviewed changes
@boydc2014
Copy link
Contributor

boydc2014 commented Apr 2, 2020

Actually, do we really need this feature? can we just remove this feature @a-b-r-o-w-n @cwhitten? like in Azure Portal to config a DB or something or LUIS key, they don't really hide the keys through some toggle control, i guess their treatment must meet compliance, so?

@a-b-r-o-w-n
Copy link
Contributor

a-b-r-o-w-n commented Apr 2, 2020

I think this was requested by the ABS-H team, I'll defer to @cwhitten on what to do.

@VanyLaw
Copy link
Contributor Author

VanyLaw commented Apr 3, 2020

@a-b-r-o-w-n @boydc2014 I think we can remove this feature too. I am not sure whether it still need after decoupled with absh publish.

@boydc2014
Copy link
Contributor

boydc2014 commented Apr 3, 2020

@cwhitten any thoughts?

@cwhitten
Copy link
Member

cwhitten commented Apr 3, 2020

@VanyLaw lets just remove the view complexity toggling this altogether for the time being.

a-b-r-o-w-n
a-b-r-o-w-n suggested changes Apr 6, 2020
View reviewed changes
Copy link
Contributor

@a-b-r-o-w-n a-b-r-o-w-n left a comment

Choose a reason for hiding this comment

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

If we are removing the ability to obfuscate the json, let's clean up the rest of the code. We can always refer back to git history to see implemenation.

@VanyLaw
Copy link
Contributor Author

VanyLaw commented Apr 6, 2020

OK, I will remove the toggle and obfuscate feature.

@a-b-r-o-w-n a-b-r-o-w-n added the Approved to merge approved, waiting to be merged label Apr 8, 2020
@a-b-r-o-w-n a-b-r-o-w-n merged commit 59a2631 into master Apr 8, 2020
@a-b-r-o-w-n a-b-r-o-w-n deleted the wenyluo/fix branch April 8, 2020 15:30
@a-b-r-o-w-n a-b-r-o-w-n mentioned this pull request May 15, 2020
Merged
lei9444 pushed a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
@VanyLaw @a-b-r-o-w-n
chore: remove show values toggle in settings page (microsoft#2476)
706fd45 
* fix toggle on change

* add debounce in setting

* change jsonEditor about obfuscate

* remove toggle in setting page

* remove obfuscate feature in jsonEditor

* fix settings page height

* memory the onChange function

* change useMemo

Co-authored-by: Andy Brown <asbrown002@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@boydc2014 boydc2014 Awaiting requested review from boydc2014 boydc2014 is a code owner

@corinagum corinagum Awaiting requested review from corinagum

@cwhitten cwhitten Awaiting requested review from cwhitten cwhitten is a code owner

1 more reviewer

@a-b-r-o-w-n a-b-r-o-w-n a-b-r-o-w-n approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Approved to merge approved, waiting to be merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@VanyLaw @boydc2014 @a-b-r-o-w-n @cwhitten