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

initial PGML version of niceTables #1034

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

drdrew42
Copy link
Sponsor Member

@drdrew42 drdrew42 commented Mar 9, 2024

With the recent updates to support tables natively in PGML (thanks @Alex-Jordan!), I'm finally able to get MultiAnswer PopUps going inside a table (e.g. columns in a truth table).

I wrote up some helper functions to take the (soon to be) old-school niceTable arguments and create a PGML table from them. This PR is the collection of those helper functions wrapped up into a single PGMLTable()object.

I tested this code out on all of the examples from our niceTables documentation, and it's mostly successful. There are a few unsupported arguments (tex and midrules, I'm looking at you), which we might consider supporting in the PGML tables code?

A new option is added, layout => 1 puts the table into LayoutTable spacing (the starred version of PGML tables).

This is just a draft PR for now, of course documentation is still needed here.

@Alex-Jordan
Copy link
Contributor

As with several other things, you might thank me for getting it started, but @dpvc did the critical work for getting that actually working.

@Alex-Jordan
Copy link
Contributor

Does it do anything for you that there is already the option for LaYoUt => 1 in niceTables table? A LayoutTable() is already just a DataTable() with LaYoUt => 1. Sorry for the camelcase, it was just supposed to be an internal thing and at the time that is how I marked something as internal.

@drdrew42
Copy link
Sponsor Member Author

drdrew42 commented Mar 10, 2024

This macro is independent of niceTables, my aim was to instead create an easy replacement for those who might be used to writing (or may be converting from) niceTables in PG. PGMLTable() takes in the same data structures as DataTable and LayoutTable, and should create identical results. Since I'm not actually using niceTables, and moreover the LaYoUt flag "shouldn't" be in anyone's code, I think it's ¯\_(ツ)_/¯

I'm sure there are others, but what I've noticed so far is that PGML ([# ... #]) tables have changed the midrules flag to horizontalrules, midrule (singular) is not recognized at the cell level, and neither is tex. [Edit: I see now that tex is deprecated 🥳] [Edit #2: okay, so documentation for niceTables is that midrules isn't the right flag anymore either -- it seems that our example problems are actually what need to be updated]

Something else to consider with tables is that <colgroup> does not apply css styling [ref], and seems to be essentially deprecated. See the last table in example 1 here, and note that and are not considered children of the and thus do not inherit the font-size attributes. This means we should rethink the way that columnscss is handled.

@Alex-Jordan
Copy link
Contributor

I see that you saw some things :) Observations you have made are not from when PGML started using niceTables, rather they were part of a cleanup that happened to niceTables a bit earlier.

The tex option is deprecated:

=item * A cell can have C<tex =E<gt> commands>.

I think instead, you now are asked to use texalignment to accomplish what you might have previously put in tex.

And midrules is classified as a "legacy misnomer". It made more sense when I had it set up to always print the top and bottom rule for the table.

$outHash{horizontalrules} = $userOptions{midrules}

The horizontalrules option will be set if midrules is attempted.

And then midrule is also a "legacy misnomer". Now the thing to do is give the individual cell a top or bottom. I'm guessing that top only works for a cell in the top row and you would use bottom most of the time in the appropriate cell. It looks like nothing is in place to convert a use of midrule to a bottom, and we could do that.

The current documentation for niceTables tells you precisely which CSS can be used for columns:

Note: only four css properties apply to a col element:

So that example from the wiki is just yet another example that needs to be updated. I think this is more about CSS that works with an HTML col, not really about colgroup. But we probably don't need to be using colgroup at all, I just went for maximal structure just in case.

I don't have it in me to maintain the wiki examples with all the breakdown and commentary, but if we maintained actual pg file examples for tables, I could update those.

If you haven't seen this yet, in develop, in the problem editor, click the PGML help button. There is some helpful stuff there about tables in PGML. It all comes from niceTables and should be a complete list of the current non-deprecated options.

@drdrew42
Copy link
Sponsor Member Author

Thanks for that -- yeah, yesterday I spent a good amount of time trying to track down what all is handled in PGML now and comparing it to the actual documentation for niceTables (rather than just trying to convert old code into this new structure).

As for PGML documentation, are you talking about the PGML-lab? I'm on develop and I don't see anything in there about tables -- am I missing something? (I don't see any POD for PGML.pl either)

@Alex-Jordan
Copy link
Contributor

Yeah, PGML lab.

So if you have an old course, it has not had the PGML lab files updated most likely. Create a new course on develop and/or copy the current modelCourse PGML-lab.pg file into your course.

@Alex-Jordan
Copy link
Contributor

This highlights an issue with PGML-lab.pg. If a course propagates over time, its PGML-lab.pg file will be relevant to the old version of WW/PG where the course was initialized, instead of whatever version the server is currently at.

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.

2 participants