Openlibrary: Create a new list button is not working

[shreyansh23]

Description

What problem are we solving? What does the experience look like today? What are the symptoms?

Create a new list button is not working (nothing is happening on clicking). I haven't used this feature before. But surely, something is wrong in this.

Relevant url?

https://openlibrary.org/books/OL23114970M/Blade_of_fire

Screenshot (if possible):

Details

Logged in (Y/N)?
Y

Browser type/version?
Firfox

Operating system?
Ubuntu 18.04

Stakeholders

@ tag stakeholders of this bug / feature
@mekarpeles

[jdlrobson]

This looks like a jQuery.live issue. It has the same symptoms as #1961

[shreyansh23]

I will try to fix this.

[shreyansh23]

@jdlrobson
I tracked this regression down to a commit. This was broken in f9905410. Before this the list creation is working correctly. I reverted the changes done in that commit (on top of current master) and everything is working fine.
By adding some console.log statements, I deduced that it is because of e.stopPropagation(); in $('#widget-add').on('click', function(e) { console.log("f-2") e.stopPropagation(); });

[shreyansh23]

@jdlrobson
I tracked this regression down to a commit. This was broken in f9905410. Before this the list creation is working correctly. I reverted the changes done in that commit (on top of current master) and everything is working fine.
By adding some console.log statements, I deduce that it is because of e.stopPropagation(); in $('#widget-add').on('click', function(e) { console.log("f-2") e.stopPropagation(); });

Should I revert that particular commit?

[mekarpeles]

@shreyansh23 I think's its safe to just commit on top of whatever is on master.

[shreyansh23]

Okay @mekarpeles. I will do it.

[shreyansh23]

I reverted the changes but I am getting an error in running docker-compose exec web bash make js

I suppose it is related to Webpack.
How can I fix that?
@mekarpeles @jdlrobson

[jdlrobson]

We don't want to do a full revert as that's going to break the problem that patch was fixing. Do you know which line the problem is on?

My guess is the list shows but the event is propogated and there is a click handler related to the list item that needs to call ev.stopPropagation.
You can put a console.log inside the document click handler to verify this theory one way or the other.

[shreyansh23]

Yes, I also suppose that e.stopPropagation is preventing other event handlers.

[shreyansh23]

@jdlrobson
Still, please check the webpack error. Getting error when using make js command.

[mekarpeles]

@shreyansh23 I suspect (because webpack was added very recently) that it is not installed within your instance of docker. You should be able to docker exec into oldev and npm install webpack

[mekarpeles]

Sorry this happened!

[jdlrobson]

Okay. Took a look at this.
The handler for creating a list appears to live inside openlibrary/templates/lists/widget.html and look like this

$("#create-new-list").click(function() {
           close_popup();
           $(".listClick").click();
});

widget-add stops event propagation and listClick is a child of this element so you are right to point out this patch as breaking it (but it's a red herring!).

The real problem from my perspective here is the line $(".listClick").click(); is relying on event propagation and that's very fragile.

Instead I would expect something like this:

$("#create-new-list").click(function() {
           close_popup();
           show_create_list_popup();
});

The question remains, where is the code that creates the popup for creating a list?

Could it be substituted for the more explicit:

$("#create-new-list").click(function() {
           close_popup();
          $.colorbox({inline: true, opacity: "0.5", href: "#addList"});
} );

[shreyansh23]

@jdlrobson

I think I partially didn't get what you were trying to convey. I suppose it is because I am not able to understand what f9905410 is doing. It is titled "Cleanup/document dropclicker behaviour". But I think you were doing something more than that. Can you tell me what is your objective with that commit?

[jdlrobson]

As I said in https://github.com/internetarchive/openlibrary/issues/1970#issuecomment-475095252 the issue is with the html template not the JS. Reverting that patch brings other problems.

[jdlrobson]

The list overlay is now showing but I get a JS error when I try to create:

Uncaught ReferenceError: ungettext is not defined