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.

Sign up for GitHub

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

Jump to bottom

Validator displays name fields from error path #72

Merged
merged 5 commits into from
Aug 5, 2024
Merged

Validator displays name fields from error path #72

merged 5 commits into from
Aug 5, 2024

Conversation

b-wils
Copy link
Contributor

@b-wils b-wils commented Aug 2, 2024

No description provided.

@b-wils b-wils requested review from fmalita, mbasaglia and Aidosmf August 2, 2024 17:03
@mbasaglia
Copy link
Member

I'm not too sure about this:

  • The code style doesn't match the rest of validator.js
  • It's display code so it should probably go into validator/index.md (validator.js is what will become a stand-alone library)
  • If the JSON is missing mn in one of the objects in the path, the path becomes a bit misleading

@b-wils
Copy link
Contributor Author

b-wils commented Aug 5, 2024

  • Is there a style guide or summary somewhere? I can take another pass to try to infer and match, but it's quite a bit different from what we use.
  • That's a good point, the joining is more display but I think the extraction into an array would be be usable. I can make that change.
  • I'm not sure I follow the mn concern. Would adding a default value in cases where it is missing help fix that?

@mbasaglia
Copy link
Member

Regarding mn, if you have layer with mn: Layer1, then a group shape without mn, and a shape with mn: Ellipse, it will show Layer1 > Ellipse which is a bit misleading since there one more step between them

@b-wils
Copy link
Contributor Author

b-wils commented Aug 5, 2024

Addressed the concerns. Style should be closer, but let me know if there are still mismatches.

mbasaglia
mbasaglia reviewed Aug 5, 2024
View reviewed changes
docs/static/js/validator.js Outdated
.map(e => {
return {
...e,
path_names: this._get_friendly_path_names(e.path, data)
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth adding to _cleaned_error to avoid a second pass with map()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@b-wils b-wils requested a review from mbasaglia August 5, 2024 17:37
@b-wils b-wils merged commit ffa3bf6 into lottie:main Aug 5, 2024
3 checks passed
b-wils added a commit to b-wils/lottie-spec that referenced this pull request Nov 25, 2024
@b-wils
Validator displays name fields from error path (lottie#72)
8bf3db9 
* Validator displays name fields from error path

* Validtor layer name extraction improvements

* Move the name joining into md file
* Add null names for layers without names

* Styling fixes to make it more consistent

* Fix variable naming to underscore case

* Refactor path name extraction into error_clean function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbasaglia mbasaglia mbasaglia approved these changes

@fmalita fmalita Awaiting requested review from fmalita

@Aidosmf Aidosmf Awaiting requested review from Aidosmf

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@b-wils @mbasaglia