-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Added feature to indicate the user if the content JSON is exploitable… #366
base: main
Are you sure you want to change the base?
Added feature to indicate the user if the content JSON is exploitable… #366
Conversation
@TallTed I have added changes as per your suggestions. |
@TallTed The improvements have been added, I had accidentally clicked on the close button, but now the pr is open again. |
@tusharkc — I'm just a contributing community member. My coding language is English, and I mostly poke at human-facing things. I have no other involvement in this project. |
src/containers/Editor/BottomBar.tsx
Outdated
@@ -114,11 +115,16 @@ export const BottomBar = () => { | |||
const getFormat = useFile(state => state.getFormat); | |||
const [isPrivate, setIsPrivate] = React.useState(false); | |||
const [isUpdating, setIsUpdating] = React.useState(false); | |||
const contents = useFile(state => state.contents); |
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.
Each time the contents in updated it will cause a rerender which is a performance concern for us. Instead we could make our calculations with in useFile(state => ...)
to prevent rerender and only when the value is changed, referred as exploitable.
src/containers/Editor/BottomBar.tsx
Outdated
{isNotExploitableJson(contents) && ( | ||
<StyledBottomBarItem> | ||
<AiOutlineSafetyCertificate /> | ||
JSON is not exploitable | ||
</StyledBottomBarItem> | ||
)} |
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's better to show the warning when it's exploitable instead of not exploitable.
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.
Also we could use "Exploitable" instead of "JSON is exploitable" so it will consume less space.
Good job, I've added a few comments! |
@AykutSarac I will work on these issues now. |
here is the pr with the changes |
Feature : #279
Under this PR, I have created a function that will check if the JSON is exploitable or not, and if it is not exploitable, a new indication has been added to the bottom bar component, that will indicate that JSON is 100% safe.