[PULL REQUEST] Pull request for branch stephenfin/the-great-bootstrapification
Daniel Axtens
dja at axtens.net
Wed Oct 31 00:14:20 AEDT 2018
Stephen Finucane <stephen at that.guru> writes:
> On Sun, 2018-10-28 at 23:57 +1100, Daniel Axtens wrote:
>> Stephen Finucane <stephen at that.guru> writes:
>>
>> > On Thu, 2018-10-18 at 14:34 +1100, Daniel Axtens wrote:
>> > > Hi Stephen,
>> > >
>> > > > Time to switch to Bootstrap 4. I've sent this as a pull request due to
>> > > > the size of the some of the files (if various SMTP servers didn't drop
>> > > > them, the mailing list would). However, there isn't a whole lot going
>> > > > on here. I mostly update all the dependencies I can to the latest
>> > > > version and then make the minimal amount of changes possible to handle
>> > > > the Bootstrap 3 -> 4 migration.
>> > > >
>> > > > Please reply with comments here or against the patch on my GitHub. If
>> > > > necessary, I can send this to individuals, but I'll have to do so in
>> > > > Base64 encoding to work around line length problems (so you won't be
>> > > > able to review this via email).
>> > > >
>> > >
>> > > I tried to pull this but it clashes with the filter changes. Would you
>> > > mind please rebasing it?
>> >
>> > Done and pushed to the same branch.
>>
>> Thanks, and thanks for keeping us up to date.
>
> And thanks for the review. I can address almost all of the below (see
> below) but I think this is the first time I've used 'git request-pull'.
> What's the etiquette here w.r.t. updates? Do I force push over my
> current branch and resend this mail? Push to a new branch? Push
> separate fixup commits onto the current branch? Something else
> entirely?
I am not entirely sure. The key bit of bad behaviour to avoid in
kernel-land is to rebase a branch that people expect to be stable. (As a
consequence it's conventional to clearly indicate which branches are
prone to being rebased!). Kernel-land also tries to avoid fixup
patches. I *think* what would happen in kernel-land is that a v2 pull
request would be sent with a different branch. But hey, we're not the
kernel and we don't have a large community, so if you just want to
force-push the old branch I'm totally OK with that.
>> I had been meaning to do
>> reintroduce the selenium tests to test functional equivalence and update
>> jQuery but I had completely forgotten that bootstrap has versions too!
>
> Go for it. Could I ask that we namespace these things though, perhaps
> by moving all the other tests into a "unit" folder and placing the
> Selenium tests in a "functional" folder? I don't personally use them so
> I'd rather no have to wrap tox like we used to. For reference, to run
> tests I currently do:
>
> docker-compose run --rm web tox -e py35-django20 {optional specifier}
Will do (eventually).
> As an aside, I've been experimenting with Vue.js lately, with the aim
> of providing a second REST-powered front-end. It's been going pretty
> well, some REST API bugs and feature gaps aside. Depending on how my
> experiments work out, this whole process could be changing at some
> point in the (distant!) future.
I am in general very much a fan of moving in this direction.
>
>> I have some thoughts. This is all based on testing in Google Chrome
>> stable, and I can send some annotated images if anything is confusing.
>>
>> Patch list page:
>>
>> - the table header is now dark (previously it was light). This shows
>> that the margin inside the table on the left side of the P in patch
>> is too small (at least on my high-dpi screen). Ideally I think I'd
>> like the colours not to change, but failing that the margin needs to
>> be bigger.
>>
>> - the table is slightly less dense, because fonts on the list page have
>> changed from 14pt Helvetica Neue. The new size is 14.4pt. The new
>> font is "-apple-system-font,...". I obviously don't have that on my
>> Linux box - so I think I'm still on Helvetica Neue. (Anyone who has
>> Roboto installed will also see a change - not sure who that is?)
>
> I think this is because they switched to rem (root em) rather than px
> or whatever they were using before. I'll experiment with this but to be
> clear, I'd would really prefer to hack on Bootstrap as little as
> possible. I'm no web dev and carrying lots of our own CSS sounds like
> hell :D Also, Roboto would affect anyone on Android, FYI (Bootstrap is
> mobile first).
Sure, let's get the main migration sorted first. Once the big ticket
items work, I'm happy to see if there's a nice minimal set of local
changes that we can carry in an isolated CSS file. I once did a very
very small amount of front-end work - my IT consultancy was a
single-person undertaking, so there was no-one else to do it -
so I can take this on.
>> - The click to copy patch IDs (when enabled) have lost a bunch of their
>> padding (top and bottom) and generally look weird and a bit broken.
>>
>> - all the colours are much more vivid, the colour change on hover is
>> stronger, etc. I prefer the more muted pallete but I wouldn't block
>> on it if it's a pain to change.
>
> This is another one thing I'd rather not change if it's a lot of work,
> even though I do agree with it in principle. Let me experiment.
As per the above, if it isn't trivial I'm happy to take a crack at it.
>> - somehow long submitter names are wrapping where previously they
>> didn't. I don't have an exact reason or reproducer, but I suspect the
>> font size changes are to blame.
>>
>> - the page and table header now sticks to the top of the screen when
>> scrolling. I don't know for sure, but I reckon it's pretty likely
>> that this will irritate some regular users. (It's probably also not
>> *that* useful as you're probably not super likely to click those
>> links often while interacting with patchwork?)
>
> The page header was intentional, based on the design of other sites. I
> can revert this though. I'm pretty sure the table header was always
> sticky though?
You are right, I hadn't realised. Let's revert the page header and keep
the table header.
>
>> Patch detail page:
>>
>> - the patch author bar (just under "Commit Message") is too close to
>> the text.
>>
>> - the text has been unindented and is now too close to the left side of
>> the screen.
>>
>> - the text has shrunk and is now IMO uncomfortably small. Chrome's
>> inspector tool has it going from 13pt to 12.6pt and it's amazing what
>> difference 0.4pt makes!
>>
>> - these also affect every comment.
>>
>> - the patch buttons (patch ID, diff, mbox, series) have all switched to
>> white on dark grey. idk how I feel about it but I want to highlight
>> it in case anyone else has thoughts.
>>
>> - the page header sticks to the top of the screen when scrolling, and I
>> think that as with the patch list page this probably isn't super
>> helpful.
>>
>> Functionality:
>>
>> - the autocomplete sender lookup box seems to work, as does
>> copy-to-clipboard (and filtering generally).
>
> Hurrah! Finally :)
>
>> - *IMPORTANT* checkboxes are kind of broken: you can no longer click
>> one and then shift-click another patch above or below to select all
>> patches in the range.
>
> Again? There seems to be an issue with clickboxes.js and jQuery 3.x.
> I'm probably going to end up having to re-implement this, I guess,
> assuming we want jQuery 3.x :( As an aside, why can't people version
> stuff properly? checkboxes.js clearly lists jQuery 3.x as supported
> </rant>
>
>> - I see a transient js error on the console when I click a link from
>> the patch list page, but it doesn't seem to matter and could be
>> internal to something unrelated to us?
>
> About changing the DOM while scrolling? I saw that too. I think it's
> Bootstrap itself that's raising it. Seemed like a non-issue though.
>
>> - Show/hide headers/series seems to work.
>>
>> - bundle drag-and-drop reordering works (I have been a patchwork
>> maintainer for I forget how long and I have only just discovered this
>> page and this feature. Huh. I should check that my patch-id to msg-id
>> migration doesn't break it.) The delete confirmation doesn't work but
>> I don't think that's the fault of this patch set.
>>
>> Misc:
>>
>> - When creating a bundle, the top of the resultant status message gets
>> hidden by the top menubar.
>
> Good catch. I need to rework the notification system at some point.
>
>> - There's way too much whitespace between the status message after
>> creating a bundle and the content below it.
>>
>> Thanks again for your work keeping us up to date!
>
> All good. Web dev is the wild west and I dislike it. Long live backend
> dev.
Yep, I agree, backend is much nicer.
Regards,
Daniel
>
> Stephen
>
>> Regards,
>> Daniel
>>
>> >
>> > Stephen
>> >
>> > > Regards,
>> > > Daniel
>> > >
>> > > > Cheers,
>> > > > Stephen
>> > > >
>> > > >
>> > > > The following changes since commit ae154148c78a75ff73c3c22f0ff0c6b3a3d01408:
>> > > >
>> > > > templates: Avoid recursive call (2018-10-01 22:49:51 +0100)
>> > > >
>> > > > are available in the Git repository at:
>> > > >
>> > > > https://github.com/stephenfin/patchwork the-great-bootstrapification
>> > > >
>> > > > for you to fetch changes up to 6bfd03293add68db8b8de01595f94266efdd2643:
>> > > >
>> > > > htdocs: Remove glyphicons (2018-10-01 23:08:45 +0100)
>> > > >
>> > > > ----------------------------------------------------------------
>> > > > Stephen Finucane (10):
>> > > > htdocs: Move all jQuery files from 'lib'
>> > > > htdocs: Fix formatting issues with README
>> > > > htdocs: Add and integrate Font Awesome
>> > > > htdocs: Update checkboxes.js to v1.2.2
>> > > > htdocs: Update StickyTableHeaders to 0.1.24
>> > > > htdocs: Update jQuery to v3.3.1
>> > > > htdocs: Update selectize.js to v0.12.4
>> > > > templates: Upgrade to Bootstrap 4
>> > > > htdocs: Update Bootstrap to v4.1.3
>> > > > htdocs: Remove glyphicons
>> > > >
>> > > > htdocs/README.rst | 57 +-
>> > > > htdocs/css/bootstrap.min.css | 8 +-
>> > > > htdocs/css/bootstrap.min.css.map | 1 +
>> > > > htdocs/css/fontawesome.min.css | 5 +
>> > > > htdocs/css/selectize.bootstrap3.css | 401 ----
>> > > > htdocs/css/selectize.bootstrap4.css | 376 +++
>> > > > htdocs/css/solid.min.css | 5 +
>> > > > htdocs/css/style.css | 63 +-
>> > > > htdocs/fonts/glyphicons-halflings-regular.eot | Bin 20335 -> 0 bytes
>> > > > htdocs/fonts/glyphicons-halflings-regular.svg | 229 --
>> > > > htdocs/fonts/glyphicons-halflings-regular.ttf | Bin 41280 -> 0 bytes
>> > > > htdocs/fonts/glyphicons-halflings-regular.woff | Bin 23320 -> 0 bytes
>> > > > htdocs/js/bootstrap.min.js | 11 +-
>> > > > htdocs/js/bootstrap.min.js.map | 1 +
>> > > > htdocs/js/jquery-1.10.1.min.js | 1 -
>> > > > htdocs/js/jquery-3.3.1.min.js | 2 +
>> > > > htdocs/js/jquery.checkboxes-1.0.6.min.js | 1 -
>> > > > htdocs/js/jquery.checkboxes-1.2.2.min.js | 1 +
>> > > > htdocs/js/jquery.stickytableheaders.min.js | 7 +-
>> > > > htdocs/js/jquery.tablednd.js | 315 ++-
>> > > > htdocs/js/popper.min.js | 5 +
>> > > > htdocs/js/selectize.min.js | 7 +-
>> > > > htdocs/webfonts/fa-solid-900.eot | Bin 0 -> 180720 bytes
>> > > > htdocs/webfonts/fa-solid-900.svg | 2444 ++++++++++++++++++++
>> > > > htdocs/webfonts/fa-solid-900.ttf | Bin 0 -> 180500 bytes
>> > > > htdocs/webfonts/fa-solid-900.woff | Bin 0 -> 86876 bytes
>> > > > htdocs/webfonts/fa-solid-900.woff2 | Bin 0 -> 67400 bytes
>> > > > lib/packages/.gitignore | 1 -
>> > > > lib/packages/jquery/README | 16 -
>> > > > lib/packages/jquery/jquery-1.10.1.min.js | 6 -
>> > > > lib/packages/jquery/jquery.checkboxes-1.0.6.min.js | 1 -
>> > > > .../jquery/jquery.stickytableheaders.min.js | 1 -
>> > > > lib/packages/jquery/jquery.tablednd.js | 314 ---
>> > > > patchwork/filters.py | 33 +-
>> > > > patchwork/templates/patchwork/about.html | 32 +-
>> > > > patchwork/templates/patchwork/bundles.html | 11 +-
>> > > > .../patchwork/partials/download-buttons.html | 14 +-
>> > > > .../templates/patchwork/partials/filters.html | 37 +-
>> > > > .../templates/patchwork/partials/patch-list.html | 36 +-
>> > > > patchwork/templates/patchwork/projects.html | 42 +-
>> > > > patchwork/templates/patchwork/submission.html | 4 +-
>> > > > templates/base.html | 169 +-
>> > > > 42 files changed, 3415 insertions(+), 1242 deletions(-)
>> > > > create mode 100644 htdocs/css/bootstrap.min.css.map
>> > > > create mode 100644 htdocs/css/fontawesome.min.css
>> > > > delete mode 100644 htdocs/css/selectize.bootstrap3.css
>> > > > create mode 100644 htdocs/css/selectize.bootstrap4.css
>> > > > create mode 100644 htdocs/css/solid.min.css
>> > > > delete mode 100644 htdocs/fonts/glyphicons-halflings-regular.eot
>> > > > delete mode 100644 htdocs/fonts/glyphicons-halflings-regular.svg
>> > > > delete mode 100644 htdocs/fonts/glyphicons-halflings-regular.ttf
>> > > > delete mode 100644 htdocs/fonts/glyphicons-halflings-regular.woff
>> > > > create mode 100644 htdocs/js/bootstrap.min.js.map
>> > > > delete mode 120000 htdocs/js/jquery-1.10.1.min.js
>> > > > create mode 100644 htdocs/js/jquery-3.3.1.min.js
>> > > > delete mode 120000 htdocs/js/jquery.checkboxes-1.0.6.min.js
>> > > > create mode 100644 htdocs/js/jquery.checkboxes-1.2.2.min.js
>> > > > mode change 120000 => 100644 htdocs/js/jquery.stickytableheaders.min.js
>> > > > mode change 120000 => 100644 htdocs/js/jquery.tablednd.js
>> > > > create mode 100644 htdocs/js/popper.min.js
>> > > > create mode 100644 htdocs/webfonts/fa-solid-900.eot
>> > > > create mode 100644 htdocs/webfonts/fa-solid-900.svg
>> > > > create mode 100644 htdocs/webfonts/fa-solid-900.ttf
>> > > > create mode 100644 htdocs/webfonts/fa-solid-900.woff
>> > > > create mode 100644 htdocs/webfonts/fa-solid-900.woff2
>> > > > delete mode 100644 lib/packages/.gitignore
>> > > > delete mode 100644 lib/packages/jquery/README
>> > > > delete mode 100644 lib/packages/jquery/jquery-1.10.1.min.js
>> > > > delete mode 100644 lib/packages/jquery/jquery.checkboxes-1.0.6.min.js
>> > > > delete mode 100644 lib/packages/jquery/jquery.stickytableheaders.min.js
>> > > > delete mode 100644 lib/packages/jquery/jquery.tablednd.js
>> > > >
>> > > > _______________________________________________
>> > > > Patchwork mailing list
>> > > > Patchwork at lists.ozlabs.org
>> > > > https://lists.ozlabs.org/listinfo/patchwork
More information about the Patchwork
mailing list