swup / docs

Official swup documentation 📘
https://swup.js.org/getting-started
12 stars 35 forks source link

Optimize Events Table #117

Closed hirasso closed 1 year ago

hirasso commented 1 year ago

Closes #115

Description

Better to try it out then discussing at length :)

Converted the table of events to a linkable list. Already nice to be able to link to similar events from the list itself (e.g. "[...]differs from pageLoaded [...]")

Direct Link: https://deploy-preview-117--splendorous-kataifi-9ae281.netlify.app/events/#list-of-all-events

netlify[bot] commented 1 year ago

Deploy Preview for splendorous-kataifi-9ae281 ready!

Name Link
Latest commit 3301b8835e62ecdea2c756d17aa57b4c4afc043f
Latest deploy log https://app.netlify.com/sites/splendorous-kataifi-9ae281/deploys/64711e33b67d8d00080f8aa0
Deploy Preview https://deploy-preview-117--splendorous-kataifi-9ae281.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

daun commented 1 year ago

Something went wrong here with the table of contents. Looks like it's (partially) sorted alphabetically?

hirasso commented 1 year ago

Right! Didn't even look at the TOC 😅 ...if you like the general idea, I'll look into optimizing the TOC (also with overflow, just like the main nav)

daun commented 1 year ago

I'm not sure. I like the idea of making them linkable, but it doesn't look ... great. Having them in a compact table helped with orientation as you could see all of them at once.

hirasso commented 1 year ago

I'm not sure. I like the idea of making them linkable, but it doesn't look ... great

I see. More often then not I find myself browsing API documentation on my mobile phone rather than on a desktop computer these days, for example when I hang out at the swing on the playground... 😅 The proposed solution in this PR is just much more usable on smaller screens IMHO. So in this case, I like it a lot.

Having them in a compact table helped with orientation as you could see all of them at once.

Usually, seeing everything at once tends to confuse me more than help me with orientation 😄

@gmrchk, do you have an opinion about this?

hirasso commented 1 year ago

Sorry, this last commit slipped in – no biggie to re-create it when this doesn't get merged.

daun commented 1 year ago

Would a responsive table be an alternative? I.e. collapsing it on small screens? Not sure if this can work across all tables in the docs, though.

@media screen and (max-width: 45em) {
    tr, th, td {
        display: block;
    }

    tr {
        padding: 1em;
        border-top: 0 none;
    }

    th {
        padding: 0;
    }

    td {
        padding: 1em 0 0;
    }
}
hirasso commented 1 year ago

What about being able to link directly to certain events? I can well imagine that this would be useful in quite a few support cases.

daun commented 1 year ago

True, it won't help with that ðŸŦ 

hirasso commented 1 year ago

Maybe you are irritated by the code elements inside the headings? Or is it really the table?

daun commented 1 year ago

I just prefer the compact display. The suggested version feels like we're wasting space.

Screenshot 2023-05-26 at 15 41 47 Screenshot 2023-05-26 at 15 41 45

daun commented 1 year ago

The code elements are great.

For me, the ideal solution would be a table that collapses on mobile and has defined anchors for linking. That would probably require a custom markdown plugin, though.

Then again, I think the current table is fine. Let's see if Georgy has an opinion on this!

gmrchk commented 1 year ago

I also prefer the table view, to be honest; it's just much better structured and clear from that. Tables are terrible on mobile, so I also always love it when the table is well-reformatted for mobile into a better scrollable row-as-a-block thing, which is probably what the @daun's CSS is doing? But hey, it's not like we can't have both. Tables can have elements with id in them too, and a bit of JS can turn things into links, as it does for headings. Like @daun said, it would probably require some markdown parsing to automate, but this is probably the only table in the docs, or one of few. Maybe worth just adding the <div id="..."></div> manually in each row for this exception?

hirasso commented 1 year ago

Ok, you win 😄 ...I'll see what can be done so that we keep the table on desktop.

hirasso commented 1 year ago

I'll stay on this PR, since there where some other improvements made for the TOC as well.

hirasso commented 1 year ago

I have created an eleventy transform that injects anchor links in the events table with minimal adjustments required in the events.md file. It's basically just a wrapping div[data-table-with-anchor-links] around the table, everything else is being injected at build time. I also implemented the suggested table reset for small screens.

hirasso commented 1 year ago

What do you guys think of the new solution?

daun commented 1 year ago

Oh, the new modifications went past my radar. I think we actually landed on the perfect solution here! Works great on mobile too. Is the current state fine for you?

daun commented 1 year ago

I'll approve this just-in-case ðŸŠī

hirasso commented 1 year ago

Yes I like it a lot as well :) Will merge now