FEATURE: Add fullscreen-tables to post (PR #14709)



Overflown tables will have a “expand table” option added to open x table in a modal


This seems a little out of place, should it be moved to a separate location?

The title of this pull request changed from “FEATURE: Add fullscreen-tables” to "FEATURE: Add fullscreen-tables to post

This seems good. One concern I have is leaking memory or elements. I think in this case the elements and events are removed after each render but could you do a memory profile in chrome just to confirm stuff isn’t sitting around if it’s re-rendered many times?

You probably don’t need this class, for styling we use btn-default. (See also /styleguide/atoms/buttons.)

I’m not sure you need to check for this class name…

html.discourse-no-touch .fullscreen-table-wrapper:hover {

So that there is no effect on mobile when tapping (or while scrolling).

I think we could have one single event listener for the whole post stream, but if nothing is leaking as @eviltrout asked, that’s fine as it is.

@eviltrout @jjaffeux in testing via DevTools I did not run into any leaks. Should be fine as is.

Looking at allocation timeline I can find a detached HTML element:

This is a very low footprint and not going to happen often, so feel free to ignore, I’m only mentioning it if you want to try to improve it, but no emergency at all.