627403 - JS Abstractions, Pop-ins and Modals (Oh My!)

Status: VERIFIED FIXED

(support.mozilla.org :: Code Quality, task)

Description

[James Socol [:jsocol, :james]]

Spun off from the comment thread on https://github.com/rlr/kitsune/commit/4b3b339ff:

James:

I also wonder if you think abstractions like $.fn.popup and $.fn.modal are worthwhile. They seem to provide a pretty clean way to handle these.

Ricky:

Paul extracted the guts of our modal code into jquery.modal.js. It looks like we took a different approach by starting with the activators (links) and those point to the modals with the data- attribute, where they start with the modals and you can pass in a selector that will activate it.

Also, a difference I see is that we depend entirely on our stylesheets for positioning and sizing, where they allow you to set them programatically. I try to keep style out of js, but I know there are some cases where it is practical or necessary to calculate your styles and set them through js.

So we can probably steal some of the ideas from their implementation to be more flexible/generic. But I am not sure that our code will become simpler than $('.activates-modal').initClickModal(); which activates most of our modals now across the site.

James:

Fair enough, thanks for that description!

Paul:

It might be worth looking into what potch is doing for AMO for comparison.

Ricky:

Yes, if there was an easy way to share js like we do python I would be a lot more inclined towards changing our code to use theirs. Paul, I know you looked at all the modal code when refactoring it out. Do you have any opinions on how we could improve?

Also, are you saying potch is doing something new on AMO? Or is it the two jquery plugins that James linked to above?

Paul:

It might be better to move the positioning into JS (keep all other styling in CSS), since it would also help with bug 614617, by allowing us to make smarter decisions based on window size, etc. The simplest way I can think of would be to provide an option to initClickModal() to e.g. have "smart fixed-position" or "smart center", which would do some extra stuff when windows are too small for fixed position (turn into absolute?), and center modals of variable dimensions, which can't easily be done with CSS afaict.

We only have two types of modals in terms of positioning, right? "open near where you click" (e.g. voting on questions), and "open in center of viewport"
There are also two in terms of styling: some have a heading/legend, some don't - or do they all have it now?

That said, the way you're doing it here is pretty simple and consistent, so we could just leave these changes for a separate bug and get to them later? We don't need to make it uber-perfect :)

James:

I would separate "open where you click" and "open in center of viewport" into "pop-in" and "modal", respectively. The pop-ins are, by definition, not modal.

Comment 1

[James Socol [:jsocol, :james]]

Two things that I would like to see:

* I'd like to start moving to a more fluid layout. It works really well for AMO, and our fixed-width feels old school. Calculating position in JS may help that.

* No more flashes of "hidden" content while pages are loading. There are a number of places where the content of modals pops up before the page finishes loading. Those need to be hidden by default in CSS.

Comment 2

[Ricky Rosario [:rrosario, :r1cky]]

landed on master:
https://github.com/jsocol/kitsune/commit/c9b106aec900bf1b9bf7b678d23c356324b4267c

Comment 3

[Rebecca Billings [:rbillings]]

Verified new modals in gallery, questions, kb, aoa