[Applab Datasets] Levelbuilder manifest edit page #33061
Conversation
Shows manifest, but doesn't do anything yet.
| <h1>Edit Dataset Manifest </h1> | ||
| <textarea | ||
| id="content" | ||
| ref="content" |
There was a problem hiding this comment.
Do you need both the id and ref? id should generally be avoided in React where possible.
There was a problem hiding this comment.
id should generally be avoided in React where possible.
@jmkulwik i've never heard this before -- is there an article or something you can point me to on this?
There was a problem hiding this comment.
Hmm, good question. The reasoning I've heard is since React components are intended to be reusable, IDs shouldn't be used. i.e. to make the component reusable, you'd automatically have to refactor to remove IDs. When I searched "should I use IDs in React" I got a general consensus of "no" without a specific article. This was the closest to an article I found:
https://stackoverflow.com/questions/28519023/does-the-classname-attribute-take-on-the-role-of-the-id-attribute-in-reactjs
It's demonstrated here: https://www.javascriptstuff.com/use-refs-not-ids/
Granted, we often don't use react in a reusable way in our code. But if there was a duplicate id anywhere in the DOM tree, it could cause similar problems. If we do use an ID, it should probably be pretty specific to what we're doing.
| <textarea | ||
| id="content" | ||
| ref="content" | ||
| value={JSON.stringify(this.props.libraryManifest, null, 2)} |
There was a problem hiding this comment.
Add a comment for the primitive types here
| class ManifestEditor extends React.Component { | ||
| static propTypes = { | ||
| // Provided via Redux | ||
| libraryManifest: PropTypes.object.isRequired |
There was a problem hiding this comment.
If possible, can you change this to dataLibraryManifest or something similar? "library" is starting to become an overloaded term in our codebase.
There was a problem hiding this comment.
If you feel strongly about this I can do this as a separate PR. However, libraryManifest is part of the redux DataState, so I don't think it's likely to be confused with other libraries.
There was a problem hiding this comment.
Ok. Good call. This can be a separate PR (or part of a larger refactor to remove some of those other references to library
| import ManifestEditor from '@cdo/apps/storage/levelbuilder/ManifestEditor'; | ||
|
|
||
| $(document).ready(function() { | ||
| const manifest = getScriptData('libraryManifest'); |
There was a problem hiding this comment.
how did i never know there was a helper for this?! 🤦♀
| before_action :initialize_firebase | ||
| authorize_resource class: false | ||
|
|
||
| def show_manifest |
There was a problem hiding this comment.
nit: add comments to each route that describe the URL (e.g., this one would be # GET /datasets/manifest)
| @dataset_library_manifest = @firebase.get_library_manifest | ||
| end | ||
|
|
||
| def updated_manifest |
There was a problem hiding this comment.
nit: this should be "update" instead of "updated"
| @@ -0,0 +1,9 @@ | |||
| - require 'json' | |||
There was a problem hiding this comment.
this line should be able to be deleted (now that we moved the JSON parsing from HAML to React)
This is the first in a sequence of PRs to implement a page on levelbuilder to control the dataset manifest in Firebase. Right now, the page shows the contents at https://cdo-v3-shared.firebaseio.com/v3/channels/shared/metadata/manifest in a syntax-highlighted JSON code mirror. Changes made on this page don't do anything yet.
Links
Testing story
Reviewer Checklist: