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.

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

Lazy ajax calls for virtual area (Complete) #2071

Closed

Conversation

bharath6365
Copy link
Contributor

@bharath6365 bharath6365 commented Oct 31, 2019

  1. Loads virtual area divs once the modal load is complete thereby giving the user quick access to the content they are looking for
  2. Disables the modal control buttons till all the virtual areas are loaded as clicking on save won't work till all areas are loaded

DEMO
https://www.notion.so/Demo-f3d022a569454124a9d085f8af62d69a

@bharath6365 bharath6365 changed the title Lazy ajax calls for virutal area (Incomplete) Lazy ajax calls for virtual area (Incomplete) Oct 31, 2019
@bharath6365 bharath6365 changed the title Lazy ajax calls for virtual area (Incomplete) Lazy ajax calls for virtual area Nov 5, 2019
@bharath6365 bharath6365 changed the title Lazy ajax calls for virtual area Lazy ajax calls for virtual area (In Progress) Nov 5, 2019
@bharath6365 bharath6365 changed the title Lazy ajax calls for virtual area (In Progress) Lazy ajax calls for virtual area (Complete) Nov 7, 2019
Copy link
Member

@boutell boutell left a comment

Choose a reason for hiding this comment

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

It looks like we're still bashing out a heck of a lot of jsonCalls here... isn't this equivalent to what we were doing before? I am not sure why it is faster. Is the benefit you're going for here that you get to at least see and scroll around the form sooner? You can't actually interact with it until this is done right? Is the overall time to being able to really edit and save the form faster?

My thought had been to take a simpler approach that might avoid the need for most of these changes:

Debounce the JSON API calls, tote up their arguments, send them all as one API call if there have been no more for about half a second, and implement a route on the back end that is faster because it can see all the areas being requested and loop over them.

This could be done without deeper changes I think.

But, you do seem to indicate your implementation is faster. So please clarify in what way it is faster. I could be missing something important.

Thanks, and thanks for digging in!

@@ -323,6 +323,41 @@ apos.define('apostrophe-areas', {
return editors;
};

// Loads the virtual areas inside a modal.
self.loadVirtualArea = function () {
Copy link
Member

Choose a reason for hiding this comment

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

Should be plural.

@@ -412,6 +412,7 @@ apos.define('apostrophe-modal', {
});
self.resizeContentHeight();
self.focusFirstFormElement();
apos.areas.loadVirtualArea();
Copy link
Member

Choose a reason for hiding this comment

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

Pass in self.$el so you can use find() with a suitable scope.

@@ -323,6 +323,41 @@ apos.define('apostrophe-areas', {
return editors;
};

// Loads the virtual areas inside a modal.
self.loadVirtualArea = function () {
var $virtualAreas = $('[data-virtual-area]');
Copy link
Member

Choose a reason for hiding this comment

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

No global jquery searches, should be scoped via $el.find() which should be an argument.

Copy link
Member

Choose a reason for hiding this comment

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

Actually this whole thing should be a method of apostrophe-modal since that's where the action is. Then you can enable/disable the save operation neatly rather than adding and removing classes globally.

if ($virtualAreas.length > 0) {
// Disable the modal controls till every area is loaded. Clicking on Save fails without
// loading the virtual area.
apos.modalSupport.toggleModalControls(true);
Copy link
Member

Choose a reason for hiding this comment

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

Should be a method of modals, which will be easy to call as self.toggleControls if this entire algorithm moves into a method of modals.

var area = JSON.parse(areaAttribute);
// Remove the data-attribute after extracting data.
$fieldset.removeAttr('data-virtual-area');
// Call the Virtual Area Route.
Copy link
Member

Choose a reason for hiding this comment

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

I believe that's apos.areas.action, you don't have to parse it out. A little surprised it's there in JSON, I wouldn't depend on it.

@boutell
Copy link
Member

boutell commented Nov 7, 2019

We should talk over my larger comment before you worry about my smaller notes.

@bharath6365
Copy link
Contributor Author

bharath6365 commented Nov 7, 2019

It looks like we're still bashing out a heck of a lot of jsonCalls here... isn't this equivalent to what we were doing before? I am not sure why it is faster. Is the benefit you're going for here that you get to at least see and scroll around the form sooner? You can't actually interact with it until this is done right? Is the overall time to being able to really edit and save the form faster?

My thought had been to take a simpler approach that might avoid the need for most of these changes:

Debounce the JSON API calls, tote up their arguments, send them all as one API call if there have been no more for about half a second, and implement a route on the back end that is faster because it can see all the areas being requested and loop over them.

This could be done without deeper changes I think.

But, you do seem to indicate your implementation is faster. So please clarify in what way it is faster. I could be missing something important.

Thanks, and thanks for digging in!

Hey. Sorry, the demo that I showed you might be a bit misleading. We can immediately interact with the areas as soon as the modal loads as opposed to waiting for all the areas to load before showing the modal. As far as I see it, the end goal here is to enable the user to edit content as soon as possible. The only thing that's not interactive here is the save button on the modal which loads after all the virtual areas load. My guess/belief is that this would be loaded 9 out of 10 times by the time the user finishes editing content. In my case (Clicking on Global) 84 "edit-virtual-area" calls need to be complete, so clearly this is close to being the worst-case scenario but still, we would be able to edit content in less than 2s. Let's assume the modal took 5 seconds to load for a widget before. Now with the current approach, modal loads in a second and the user can start viewing and editing content right away while the other areas (In other tabs - Arrange Fields) load in parallel. I felt it was a UX problem that could be solved directly on the front end (That's where the problem is) without involving a single line of code on the back end.

I'm updating the demo to show you that the area is interactive right away.
https://www.notion.so/Demo-f3d022a569454124a9d085f8af62d69a

Let me know if this approach works for you and then I can look into the other details you mentioned in the PR.

@boutell
Copy link
Member

boutell commented Nov 7, 2019 via email

@boutell
Copy link
Member

boutell commented Nov 11, 2019

Hi @bharath6365, please take a look at this branch. Does it work well for your use case?

https://github.com/apostrophecms/apostrophe/pull/new/fast-edit-virtual-areas

I noticed an issue saving the second time I edited a product with 500 consecutive areas in it, but I'm not sure that issue is even new or related, since I didn't really touch the saving process.

@boutell boutell closed this Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants