Feature/refactor js data table #1883
Conversation
|
There's not a lot of solid boundaries here. I feel like this just moves stuff around. Here's an example of what I think I'm looking for and it's a super easy way to cut out all the dependencies between components. Firing very simple javascript events. Let's encapsulate the table in something that just fires simple events like this. It doesn't care how that works or what, it only provides the minimum information in a simple event. Download these files. Copy those. It passes it entirely back to some other component to know how/what to do with the event. const eventData =
{
"type": "download",
"files": [ "file_a", "file_b" ]
};
const event = new CustomEvent('table_request', eventData);
table.dispatchEvent(event); |
|
|
||
| let uppy = null; | ||
|
|
||
| $(document).ready(function() { |
There was a problem hiding this comment.
Does everything need to be in document.ready? Looks like a lot of definitions and even the uppy can be declared and initialized outside (i.e., before) this event.
There was a problem hiding this comment.
It can be declared out of document ready, but should it? My thoughts are that we should make sure the document loads and is ready prior to initializing unless we have to. uppy was a 1 off due to the need to be in a global scope for the file.
Is there a good reason why it should not be in document.ready?
There was a problem hiding this comment.
It can be declared out of document ready, but should it?
Which way is more advantageous? I'd say having one const uppy is better than initializing the variable to null then modifying it after the fact, just because it's simpler. Less state to worry about.
There was a problem hiding this comment.
If you are just talking about the uppy file, then that's fine. I didn't really need to do any major updates to that file. So, to be consistent I threw most of the stuff into document.ready.
There was a problem hiding this comment.
I was talking about this specifically, but it does seem like an odd pattern.
I know that we need to do this sometime because files are accessing variables declared somewhere else, but when we don't need to do it, I'd argue that simpler is better.
There was a problem hiding this comment.
I move the stuff out of document.ready and things broke. I now remember why I put it there. There are variables that must live in the template file since the values are coming from configs and ruby is reading those values. We have to make sure those are loaded first.
|
|
||
| if(clipboard.from == clipboard.to){ | ||
| console.error('clipboard from and to are identical') | ||
| // TODO: |
There was a problem hiding this comment.
This is new functionality. I'd rather create a different ticket for this so we can get the refactor pushed out.
There was a problem hiding this comment.
we want to begin watching out for scope creep.
* Completed New File/Folder functionality migration * Completed Upload Functionality * Completed Download Functionality * Completed Copy Path Functionality Co-authored-by: Gerald Byrket <gbyrket@osc.edu>
* Completed New File/Folder functionality migration * Completed Upload Functionality Co-authored-by: Gerald Byrket <gbyrket@osc.edu>
* Completed New File/Folder functionality migration * Completed Upload Functionality * Completed Download Functionality * Completed Copy Path Functionality * Completed Copy/Move functionality Co-authored-by: Gerald Byrket <gbyrket@osc.edu>
Co-authored-by: Gerald Byrket <gbyrket@osc.edu>
Co-authored-by: Gerald Byrket <gbyrket@osc.edu>
Co-authored-by: Gerald Byrket <gbyrket@osc.edu>
Co-authored-by: Gerald Byrket <gbyrket@osc.edu>
|
Full page reload on navigation has been fixed. Now functionality is same as before. Ready for review. |
| const EVENTNAME = { | ||
| getJsonResponse: 'getJsonResponse', | ||
| reloadTable: 'reloadTable', | ||
| goto: 'goto', | ||
| }; |
There was a problem hiding this comment.
I know we had a conversation on event names and owners, but I'm now realizing that events are bi directional and that's what was tripping us up.
Maybe it's more like a file like data_table exports two things, one for the events it listens to, what you have, and the events it itself publishes.
// events this object publishes
const PUBLISHING_EVENTS = {}
// events this object listens too.
const LISTENING_EVENTS = {}
There was a problem hiding this comment.
It does seem cleaner and easier to manage. Good call.
There was a problem hiding this comment.
After looking at the code again. I already have the events broken down to class, so there is really no need to break it down any further. We are not dealing with many event names, so to break down further in this scenario, I do not believe is necessary. However, if we get to a point where that is needed, I'll address it then.
|
Also note that the tests currently failing, I don't know why. And, unfortunately, we have a merge conflict now with that json.builder. |
| <div class="col-md-3"> | ||
| <%= render partial: "favorites" %> | ||
| <%= render partial: "copy_move_popup" %> | ||
| <%= render partial: "file_action_menu" %> | ||
| <%= render partial: "default_label_error_messages" %> | ||
| <%= render partial: "favorites", :locals => { :files => @files } %> | ||
| <%= render partial: "copy_move_popup", :locals => { :files => @files } %> | ||
| <%= render partial: "file_action_menu", :locals => { :files => @files } %> | ||
| <%= render partial: "default_label_error_messages", :locals => { :files => @files } %> | ||
| </div> | ||
| <%= render partial: "files_table" %> | ||
| <%= render partial: "files_table", :locals => { :files => @files } %> |
There was a problem hiding this comment.
Try to use locals only when you need to use locals. I'd guess you only need it 1 or 2 places here.
|
Looking to see why tests are failing now. |
This project was to break out the JavaScript from within the Template Files in the Files App.
Testing on this will be simply testing all of the functionality within the files app.
┆Issue is synchronized with this Asana task by Unito