Обсуждение: Download navigation UX

On 7/6/20 8:34 AM, Jonathan S. Katz wrote:
> On 7/6/20 6:59 AM, Dave Page wrote:
>> Following a chat with Vik, here's an updated patch which fixes a couple
>> of minor issues and greys the background of the selected OS when clicked.
>>
>> I plan to commit tomorrow if there are no objections.
>
> I was out all last week, however I will review today :)

Reviewed. Per some offline conversation as well as online review, please
see v3 of the attached patch. Notable changes:

- Renamed some IDs/classes to match the current pattern. Removed /
consolidated some of the CSS
- Moved the JavaScript into the main JS file
- A few formatting changes for readability

General comments:

- There is a use of "!important" in the anchor tag text that I'm not
that happy about, but starting to dig into it, there is a global
!important that's causing it to be that way. I didn't have time to fully
dig into it, but noting for future improvements.

- I did look at it on mobile/tablet view and it seems like it "meets the
standards" that the search engines look for, knowing that even if things
are moving more "mobile first", it's not like people are doing to
download PostgreSQL to their mobile device from a web browser (but happy
to be corrected there).

Anyway, looks good!

On Mon, Jul 6, 2020 at 4:17 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:

On 7/6/20 8:34 AM, Jonathan S. Katz wrote:
> On 7/6/20 6:59 AM, Dave Page wrote:
>> Following a chat with Vik, here's an updated patch which fixes a couple
>> of minor issues and greys the background of the selected OS when clicked.
>>
>> I plan to commit tomorrow if there are no objections.
>
> I was out all last week, however I will review today :)

Reviewed. Per some offline conversation as well as online review, please
see v3 of the attached patch. Notable changes:

- Renamed some IDs/classes to match the current pattern. Removed /
consolidated some of the CSS
- Moved the JavaScript into the main JS file
- A few formatting changes for readability

General comments:

- There is a use of "!important" in the anchor tag text that I'm not
that happy about, but starting to dig into it, there is a global
!important that's causing it to be that way. I didn't have time to fully
dig into it, but noting for future improvements.

- I did look at it on mobile/tablet view and it seems like it "meets the
standards" that the search engines look for, knowing that even if things
are moving more "mobile first", it's not like people are doing to
download PostgreSQL to their mobile device from a web browser (but happy
to be corrected there).

Anyway, looks good!

We should really avoid those inline event handlers, as they will (I think) break our CSP.

I notice that some of those snuck in again in the copy-urls-to-clipboard patch of recent, and that should be fixed again. But before that we put in some work to try to avoid those, let's try not to break that. E.g. for the download pages, specifically in 1d78793add2c65545f7f37dbd75ab7decc399cea.

On 7/7/20 6:02 AM, Dave Page wrote:
> Here's an updated patch (based on Jonathan's update) which combines the
> inline handler patch and removes the inline handlers from the main patch.

Looking good. One comment:

Given there's nothing dynamic about serving the new apt.js /
download.js, we don't need to serve those dynamically from the Django
app. We can just plop them into "/media/js", e.g.
"/media/js/download.js" and then this becomes:

<script type="text/javascript"
src="/media/js/download.js?{{gitrev}}"></script>

On Tue, Jul 7, 2020 at 3:15 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:

On 7/7/20 6:02 AM, Dave Page wrote:
> Here's an updated patch (based on Jonathan's update) which combines the
> inline handler patch and removes the inline handlers from the main patch.

Looking good. One comment:

Given there's nothing dynamic about serving the new apt.js /
download.js, we don't need to serve those dynamically from the Django
app. We can just plop them into "/media/js", e.g.
"/media/js/download.js" and then this becomes:

<script type="text/javascript"
src="/media/js/download.js?{{gitrev}}"></script>

+1. And we already have a download.js there, so we should probably reuse that.

On 7/7/20 6:02 AM, Dave Page wrote:
> Here's an updated patch (based on Jonathan's update) which combines the
> inline handler patch and removes the inline handlers from the main patch.

Looking good. One comment:

Given there's nothing dynamic about serving the new apt.js /
download.js, we don't need to serve those dynamically from the Django
app. We can just plop them into "/media/js", e.g.
"/media/js/download.js" and then this becomes:

<script type="text/javascript"
src="/media/js/download.js?{{gitrev}}"></script>

On 7/7/20 11:53 AM, Dave Page wrote:
>
>
> On Tue, Jul 7, 2020 at 2:15 PM Jonathan S. Katz <jkatz@postgresql.org
> <mailto:jkatz@postgresql.org>> wrote:
>
>     On 7/7/20 6:02 AM, Dave Page wrote:
>     > Here's an updated patch (based on Jonathan's update) which
>     combines the
>     > inline handler patch and removes the inline handlers from the main
>     patch.
>
>     Looking good. One comment:
>
>     Given there's nothing dynamic about serving the new apt.js /
>     download.js, we don't need to serve those dynamically from the Django
>     app. We can just plop them into "/media/js", e.g.
>     "/media/js/download.js" and then this becomes:
>
>     <script type="text/javascript"
>     src="/media/js/download.js?{{gitrev}}"></script>
>
>
> Thanks - committed with that change.

Awesome, thanks!

On 2020-Jul-07, Dave Page wrote:

> Thanks - committed with that change.

Cool stuff, thanks, looks good!

So now we can discuss the addition of 2ndQuadrant and PostgresPro
installers there, aye?

On 2020-Jul-07, Magnus Hagander wrote:

> If you mean getting third party installers off the main download page and
> moved into the software catalogue, then yes, of course :)

Well, I meant this patch
https://postgr.es/m/CAH+GA0pUz26AoyF+LazhPS02HwuBO+kPZ2wcRcOwq9u+cPPOag@mail.gmail.com
which apparently has been waiting for this restructuring for years.
(Oleg said there's a PostgresPro patch somewhere too, but I don't know
where that is.)

On 2020-Jul-07, Dave Page wrote:

> Thanks - committed with that change.

Cool stuff, thanks, looks good!

So now we can discuss the addition of 2ndQuadrant and PostgresPro
installers there, aye?

On Tue, Jul 7, 2020 at 5:54 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:

On 7/7/20 11:53 AM, Dave Page wrote:
>
>
> On Tue, Jul 7, 2020 at 2:15 PM Jonathan S. Katz <jkatz@postgresql.org
> <mailto:jkatz@postgresql.org>> wrote:
>
>     On 7/7/20 6:02 AM, Dave Page wrote:
>     > Here's an updated patch (based on Jonathan's update) which
>     combines the
>     > inline handler patch and removes the inline handlers from the main
>     patch.
>
>     Looking good. One comment:
>
>     Given there's nothing dynamic about serving the new apt.js /
>     download.js, we don't need to serve those dynamically from the Django
>     app. We can just plop them into "/media/js", e.g.
>     "/media/js/download.js" and then this becomes:
>
>     <script type="text/javascript"
>     src="/media/js/download.js?{{gitrev}}"></script>
>
>
> Thanks - committed with that change.

Awesome, thanks!

I still stand by my review comment that it shouldn't be multiple JS files for such trivial things. 

On Tue, Jul 7, 2020 at 4:55 PM Magnus Hagander <magnus@hagander.net> wrote:

On Tue, Jul 7, 2020 at 5:54 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:

On 7/7/20 11:53 AM, Dave Page wrote:
>
>
> On Tue, Jul 7, 2020 at 2:15 PM Jonathan S. Katz <jkatz@postgresql.org
> <mailto:jkatz@postgresql.org>> wrote:
>
>     On 7/7/20 6:02 AM, Dave Page wrote:
>     > Here's an updated patch (based on Jonathan's update) which
>     combines the
>     > inline handler patch and removes the inline handlers from the main
>     patch.
>
>     Looking good. One comment:
>
>     Given there's nothing dynamic about serving the new apt.js /
>     download.js, we don't need to serve those dynamically from the Django
>     app. We can just plop them into "/media/js", e.g.
>     "/media/js/download.js" and then this becomes:
>
>     <script type="text/javascript"
>     src="/media/js/download.js?{{gitrev}}"></script>
>
>
> Thanks - committed with that change.

Awesome, thanks!

I still stand by my review comment that it shouldn't be multiple JS files for such trivial things. 

Hmm, for some reason this and a number of your other messages went into spam. I wonder what Google is trying to tell me?

They're in separate files because they are setting up event handlers specific to each page. I don't know of a way to do that with a shared file without jumping through some likely hacky and fragile hoops to allow the code to figure out what page it's been called from - i.e. making sure that certain DOM IDs are always unique across any pages the shared file is called from, and testing for the existence of those IDs to figure out what event handlers to setup at runtime.

Having the separate files is certainly an extra download and slightly less cacheable (for a few bytes of code), but I think that's outweighed by the additional complexity and code that having a single file would require.

They're in separate files because they are setting up event handlers specific to each page. I don't know of a way to do that with a shared file without jumping through some likely hacky and fragile hoops to allow the code to figure out what page it's been called from - i.e. making sure that certain DOM IDs are always unique across any pages the shared file is called from, and testing for the existence of those IDs to figure out what event handlers to setup at runtime.

We already do similar in forms.js, though I have to admit a lot less than I thought we were. I guess I'm getting it slightly confused with other repos.

We *already* have the requirement to keep "certain DOM Ids" unique -- we use it for CSS. And I really don't see that as any bigger problem than keeping the filename of the JS file unique.

Having the separate files is certainly an extra download and slightly less cacheable (for a few bytes of code), but I think that's outweighed by the additional complexity and code that having a single file would require.

It's really more the latency than anything else. I'm pretty strongly on the side of thinking the cost of that is higher than the small amount of complexity -- I really don't think it would be much. But no, I'm not strongly *enough* on that side to actually try to produce proper metrics to prove it :)

On Wed, Jul 8, 2020 at 4:16 PM Magnus Hagander <magnus@hagander.net> wrote:

They're in separate files because they are setting up event handlers specific to each page. I don't know of a way to do that with a shared file without jumping through some likely hacky and fragile hoops to allow the code to figure out what page it's been called from - i.e. making sure that certain DOM IDs are always unique across any pages the shared file is called from, and testing for the existence of those IDs to figure out what event handlers to setup at runtime.

We already do similar in forms.js, though I have to admit a lot less than I thought we were. I guess I'm getting it slightly confused with other repos.

That's a *lot* more generic though from what I can see.

We *already* have the requirement to keep "certain DOM Ids" unique -- we use it for CSS. And I really don't see that as any bigger problem than keeping the filename of the JS file unique.

Do you have an example? I can't see any obvious ones. Ideally we should be using classes for CSS imho, rather than IDs anyway imho (and we really shouldn't be using IDs in a way that requires them to be unique across pages that may well be unknown to the casual hacker - that would be a real maintenance headache).

Having the separate files is certainly an extra download and slightly less cacheable (for a few bytes of code), but I think that's outweighed by the additional complexity and code that having a single file would require.

It's really more the latency than anything else. I'm pretty strongly on the side of thinking the cost of that is higher than the small amount of complexity -- I really don't think it would be much. But no, I'm not strongly *enough* on that side to actually try to produce proper metrics to prove it :)

The code complexity isn't that much I grant you. We'd have to wrap something like the following around each event listener setup:

var elem = document.getElementById("elementID");

if (elem) {

    // setup listener 

}

However the cognitive complexity for the maintainer/developer of ensuring IDs are unique across different pages is *much* higher.

>     However the cognitive complexity for the maintainer/developer of
>     ensuring IDs are unique across different pages is *much* higher.
>
> I still don't see how those are very high. What's the likelihood of
> using an id like "yumdownloads" (or similar) on a page that is unrelated
> to yum? I'd say that's pretty much zero.

Yeah, I think the probability of this being an issue is low.