-
Notifications
You must be signed in to change notification settings - Fork 402
refactor: allow .json extension on annotations file #836 #1402
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
Merged
JosefBredereck
merged 14 commits into
pattern-lab:dev
from
mfranzke:rename-annotations-js
Jan 29, 2022
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
8e3ec8c
refactor: corrected annotations.js to annotations.json
mfranzke 41b4ec1
refactor(annotations): allow .json extension on annotations file #836
mfranzke 7a54522
refactor: corrected annotations.js to annotations.json
mfranzke dfd9e8b
feat(annotations): added at least annotation to the sample content
mfranzke 199d87d
chore: added another annotations example
mfranzke 4b34af2
refactor(docs): corrected the filetype in the docs as well
mfranzke c585da9
docs(annotations): added further details
mfranzke c16211e
Merge branch 'dev' into rename-annotations-js
mfranzke be6a8d8
Merge branch 'dev' into rename-annotations-js
mfranzke 9639276
Merge branch 'dev' into rename-annotations-js
mfranzke fb3ba22
refactor: removed this file again for the moment
mfranzke 18d475b
refactor: renamed this file (after the latest merge)
mfranzke da0fa74
Merge branch 'dev' into rename-annotations-js
JosefBredereck ed0873d
refactor: migrated that file as well
mfranzke File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 changes: 0 additions & 3 deletions
3
packages/development-edition-engine-handlebars/source/_annotations/annotations.js
This file was deleted.
Oops, something went wrong.
3 changes: 3 additions & 0 deletions
3
packages/development-edition-engine-handlebars/source/_annotations/annotations.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| { | ||
| "comments": [] | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm a bit on the fence with that. We should let the user decide if he wants to use JS or JSON, even in the future.
Furthermore, we could check if it is possible to
requirethat.jsor.jsonfile so that we do not have to strip comments and anything else in that file. Maybe we can change the way it needs to be written to amodule.exportsformat.Uh oh!
There was an error while loading. Please reload this page.
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.
@JosefBredereck, actually I've realized that we're stripping that
var commentspart first of all, to add it again on exporting the data:patternlab-node/packages/core/src/lib/exportData.js
Line 72 in 41b4ec1
So another solution might be to skip that whole transition to a pure JSON file and only provide the possibility with using JS (the existing format), but remove the JSON file possibility, as we're even already providing another format with the markdown notation as well.
So another task for this ticket could be still to correct the documentation part anyhow: https://patternlab.io/docs/adding-annotations/#heading-json-example
What do you think?
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.
No way 😄. In that case, throw away those nasty JSON files 🗑️
Uh oh!
There was an error while loading. Please reload this page.
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.
okay, sounds reasonable to me. I've set this ticket on draft again and will proceed by removing these parts as well as the nasty existing transitions in the codebase, as well as correct the documentation page.
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.
The whole file you mentioned is a solution, but if we would rework the codebase with today's standards, we would maybe not do it that way.
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.
so does it mean that you want to come up with (or at least describe) a solution ?
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.
Not for now; it would require a more significant rewrite.
This line shows that we already work with modules somewhere.
patternlab-node/packages/core/src/lib/exportData.js
Line 88 in 41b4ec1
But then we also need to adjust the place where we load those annotations again.
What makes me curious. Annotations always have a comments property and no other. Is it possible that they have different properties? If not, this would be another change I would like to implement somehow.
patternlab-node/packages/core/src/lib/exportData.js
Line 72 in 41b4ec1
Uh oh!
There was an error while loading. Please reload this page.
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.
@JosefBredereck, on the other hand, besides the strange way to include the JSON data into the website after a transformation, the positive way of using JSON (even only) would be that this adapts the way other data is being maintained in the pattern lab system (like pattern data).
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.
That is correct. Nodejs is so fast in transforming the data that we could use the approach that is already in this PR. In the end, it makes no difference.