Skip to content

Dialogs: Move click handler from templates to JS#2421

Merged
cdrini merged 1 commit intointernetarchive:masterfrom
jdlrobson:clickHandlersCloseDialog
Oct 8, 2019
Merged

Dialogs: Move click handler from templates to JS#2421
cdrini merged 1 commit intointernetarchive:masterfrom
jdlrobson:clickHandlersCloseDialog

Conversation

@jdlrobson
Copy link
Collaborator

Description

From now on simply adding a class .floaterShut to an element will make it close an open dialog.
.floaterShut--parent will request the parent element closes it.

Previously we did this via inline JavaScript which made it really hard to determine which pages should run this code. Having it in a central place also allows us in future to defer the loading of jquery ui off the critical path. This is the most simple first step!

Testing

Visit http://localhost:8080/works/OL53924W/The_complete_works_of_Mark_Twain#addImage and click "change cover". The close icon should close the dialog

@jdlrobson jdlrobson changed the title Move click handler from templates to JS Dialogs: Move click handler from templates to JS Sep 27, 2019
@jdlrobson jdlrobson force-pushed the clickHandlersCloseDialog branch from a0c2624 to 860a343 Compare September 27, 2019 23:53
@jdlrobson jdlrobson added the Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] label Sep 27, 2019
@jdlrobson jdlrobson force-pushed the clickHandlersCloseDialog branch 2 times, most recently from 91b047b to b3a3ca3 Compare September 29, 2019 00:08
@cdrini cdrini self-assigned this Oct 1, 2019
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

Overall looks good; some naming things/introducing duplicate code. I know you have dependent commits on this, so if it's easier, you can put these changes in new commits on your WIP branch and I can review that. If anything requires a ton of rebasing of your other PR, PLEASE LET ME KNOW! We can just do it there.

@cdrini cdrini added Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] and removed Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] labels Oct 2, 2019
@jdlrobson jdlrobson force-pushed the clickHandlersCloseDialog branch from b3a3ca3 to 12556c7 Compare October 4, 2019 05:17
@jdlrobson jdlrobson added Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] and removed Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] labels Oct 4, 2019
@jdlrobson
Copy link
Collaborator Author

Are you okay for me to do the remaining changes as follow ups? I should have some time available tomorrow to do that as well as get #2423 ready.

@cdrini
Copy link
Collaborator

cdrini commented Oct 4, 2019

That seems good to me. Are you going to put them on top of your WIP PR?

@jdlrobson
Copy link
Collaborator Author

I've got a new patchset locally that ill push when this is merged.

@jdlrobson
Copy link
Collaborator Author

Okay, given it's not merged I'll temporary submit to my WIP branch. When it's merged I'll split that into reviewable separate pull requests.

@jdlrobson
Copy link
Collaborator Author

For completeness the follow ups are in 00a4db2

I can push a pull request tomorrow if this one is merged tonight/tomorrow

@jdlrobson jdlrobson force-pushed the clickHandlersCloseDialog branch from 12556c7 to 29b30e8 Compare October 5, 2019 00:06
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

Tested:

@cdrini
Copy link
Collaborator

cdrini commented Oct 7, 2019

Oh, whoops, missed the other commits on here. My apologies for the delay; I had weekend plans and wasn't able to finish this before heading out of town.

@jdlrobson
Copy link
Collaborator Author

jdlrobson commented Oct 8, 2019 via email

From now on simply adding a class .floaterShut to an element
will make it close an open dialog.

.floaterShut--parent will request the parent element closes it.
@jdlrobson jdlrobson force-pushed the clickHandlersCloseDialog branch from 29b30e8 to 08bab49 Compare October 8, 2019 19:33
@jdlrobson
Copy link
Collaborator Author

Sorry for confusion let's move discussion about clickHander

Oh, whoops, missed the other commits on here. My apologies for the delay; I had weekend plans and wasn't able to finish this before heading out of town.

Sorry for confusion that wasn't intentional. We can continue the discussions on #2423 once this is merged.

@cdrini cdrini merged commit 4f8a941 into internetarchive:master Oct 8, 2019
@cdrini cdrini removed the Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] label Oct 8, 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.

2 participants