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.
Hmm, yeah - so they do.
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.