Fix automatic exporting and add missing IDs back to mapping tables #105
Conversation
spectranaut
force-pushed
the
table-ids
branch
from
e71ed47
to
5fa4b38
Compare
| @@ -103,16 +89,13 @@ function mappingTables() { | |||
|
|
|||
| // create a container div to hold all the details element and insert after table | |||
| tableInfo.detailsContainer = document.createElement('div'); | |||
| tableInfo.detailsContainer.className = 'details removeOnSave'; | |||
| tableInfo.detailsContainer.className = 'details'; | |||
There was a problem hiding this comment.
This change makes it such that if you load the html page, click the "respec" menu button, then "export", the exported document has the "summary view" instead of the tables. Is this what we want?
There was a problem hiding this comment.
I'm not sure but I think we need the tables exported, because the UI script keys of those for the swap. But if saving the summary view still preserves two-way transformation ability, and we think it's a better default presentation, then I'd say that can work.
There was a problem hiding this comment.
I think I don't quite follow what your saying here @michael-n-cooper -- what is "the UI script keys of those for the swap"?
There was a problem hiding this comment.
Sorry, I mean my understanding is the table structure is used to build two representations, one a copy of the table structure, and the other the details structure. When a user switches view, the appropriate version is written to the DOM. My understanding from my last code review is that the initial setup depends on the table structure to be present, and therefore that's what we need to export in static snapshots.
However, if that's no longer the case, and we can have a working switcher in a static snapshot that contains the details version instead of the table version, then I think it meets requirements.
There was a problem hiding this comment.
I see what you are talking about now -- the summary view is made from the table view, the table view is what you can see in the index.html file. The summary view is made when the page is loaded, and then the button to switch between the two just hides one and removes "ids", then shows the other and adds "ids". But the summary view is shown by default, so if you export the spec automatically it will export the summary view (the default) and everything will be fine. Though, the table will be in the html with display: none, if that matters.
There was a problem hiding this comment.
I see. In that instance it's probably ok, assuming the display:none does its job. :)
|
who else can review this, @jnurthen ? |
spectranaut
force-pushed
the
table-ids
branch
from
5fa4b38
to
92a1ea9
Compare
|
I just am realizing... should this PR (after approval I guess?) be moved to https://github.com/w3c/aria-common/blob/main/script/mapping-tables.js ? And does anyone else use this file besides core-aam and html-aam? |
|
i would imagine any aam (present or future) should be able to use this, if i'm not mistaken? |
|
And yes - this PR should be against aria-common but lets review it here first as its kind of hard to develop over there! |
|
@schne324 can you take a look at this pr? :) |
common/script/mapping-tables.js
Outdated
|
|
||
| var detailsHTML = '<summary id="' + id + '">' + summary; | ||
| var detailsHTML = '<summary id="' + id + '" data-id="' + id + '">' + summary; |
There was a problem hiding this comment.
adding an id and a data-id with identical values seems strange. What is the reasoning here? Maybe we should add a comment here in the code so the next person touching this code knows what is going on.
There was a problem hiding this comment.
We don't need them both at the same time! I just added a commit to make it so only one or the other exists. When converting between the "table view" and "summary view", the hidden view now has only the data-id attribute and the visible view has only an id attribute.
Does the make it less confusing?
spectranaut
force-pushed
the
table-ids
branch
from
92a1ea9
to
b7787d6
Compare
|
Can I have some reviews of this? :) |
|
Oh nice fix @michael-n-cooper I hadn't even noticed that was also broken. I opened a PR with these changes on aria-commons: w3c/aria-common#80 |
This is regard to: #102
Questions:
preProcessparameter in therespecConfig), so once i removed the respec events code -- which was causing an error -- the tables showed up twice (as mappingTables was run twice, once during preprocessing and once when mapping-tables.js was loaded).💥 Error: 500 Internal Server Error 💥
PR Preview failed to build. (Last tried on Feb 15, 2022, 11:22 AM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
🚨 Spec Generator - Spec Generator is the web service used to build specs that rely on ReSpec.
🔗 Related URL
If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.