Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize Events Table #117

Merged
merged 16 commits into from
May 31, 2023
Merged

Optimize Events Table #117

merged 16 commits into from
May 31, 2023

Conversation

hirasso
Copy link
Member

@hirasso hirasso commented May 26, 2023

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
Copy link

netlify bot commented May 26, 2023

Deploy Preview for splendorous-kataifi-9ae281 ready!

Name Link
🔨 Latest commit 3301b88
🔍 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.

@hirasso hirasso changed the title Make events linkable Convert table of events to linkable list May 26, 2023
@hirasso hirasso changed the title Convert table of events to linkable list Convert the table of events to a linkable list May 26, 2023
@hirasso hirasso requested review from daun and gmrchk May 26, 2023 12:01
@daun
Copy link
Member

daun commented May 26, 2023

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

@hirasso
Copy link
Member Author

hirasso commented May 26, 2023

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
Copy link
Member

daun commented May 26, 2023

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
Copy link
Member Author

hirasso commented May 26, 2023

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
Copy link
Member Author

hirasso commented May 26, 2023

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

@daun
Copy link
Member

daun commented May 26, 2023

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
Copy link
Member Author

hirasso commented May 26, 2023

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
Copy link
Member

daun commented May 26, 2023

True, it won't help with that 🫠

@hirasso
Copy link
Member Author

hirasso commented May 26, 2023

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

@daun
Copy link
Member

daun commented May 26, 2023

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
Copy link
Member

daun commented May 26, 2023

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
Copy link
Member

gmrchk commented May 26, 2023

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 hirasso changed the title Convert the table of events to a linkable list Optimize Events Table May 26, 2023
@hirasso
Copy link
Member Author

hirasso commented May 26, 2023

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

@hirasso
Copy link
Member Author

hirasso commented May 26, 2023

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

@hirasso hirasso removed request for daun and gmrchk May 26, 2023 16:55
@hirasso hirasso requested review from daun and gmrchk May 26, 2023 20:43
@hirasso
Copy link
Member Author

hirasso commented May 26, 2023

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
Copy link
Member Author

hirasso commented May 31, 2023

What do you guys think of the new solution?

@daun
Copy link
Member

daun commented May 31, 2023

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
Copy link
Member

daun commented May 31, 2023

I'll approve this just-in-case 🪴

@hirasso
Copy link
Member Author

hirasso commented May 31, 2023

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

@hirasso hirasso merged commit 6138a1b into master May 31, 2023
@hirasso hirasso deleted the make-events-linkable branch May 31, 2023 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert list of events from table to list
3 participants