-
Notifications
You must be signed in to change notification settings - Fork 106
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget this TODO
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
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.
Refactored event names to be defined in the related JS Class.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does seem cleaner and easier to manage. Good call.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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 think we're ready.
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