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

Fix potentially unsafe 'title' field visit #1425

Closed
wants to merge 2 commits into from
Closed

Conversation

yeze322
Copy link
Contributor

@yeze322 yeze322 commented Oct 31, 2019

Description

As reported by Hongyang, there might be console errors about null reference exception on '.title' field (cannot repro is stably). This PR defends this issue.

Task Item

Please include a link to the related issue. Ex. Closes #<issue #>

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code refactor (non-breaking change which improve code quality, clean up, add tests, etc)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Doc update (document update)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have functionally tested my change

Screenshots

Please include screenshots or gifs if your PR include UX changes.

@@ -18,7 +18,7 @@ function getLabel(data: any): string {

const labelOverrides = ConceptLabels[data.$type];

if (labelOverrides.title) {
if (labelOverrides && labelOverrides.title) {
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps we should make title required, and make this

if (labelOverirides) {
return labelOverride.title;
}
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
image
Just checked the schema, sometimes its false to indicate show nothing.

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion is we make that required? like "title: string". will that be more simple?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is designed to allow for user's to completely hide a label or description. I would like to keep it like this because it would be a nice reference in the future for when we expand the capabilities of the editor schema.

@yeze322
Copy link
Contributor Author

yeze322 commented Nov 14, 2019

included in #1529

@yeze322 yeze322 closed this Nov 14, 2019
@yeze322 yeze322 deleted the zeye/fix/title branch December 17, 2019 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants