Skip to content

[dataquery] fixes oversized table head element - #11087

Open
marandmart wants to merge 2 commits into
aces:mainfrom
marandmart:dataquery_scrolling_query
Open

[dataquery] fixes oversized table head element#11087
marandmart wants to merge 2 commits into
aces:mainfrom
marandmart:dataquery_scrolling_query

Conversation

@marandmart

Copy link
Copy Markdown
Contributor

Brief summary of changes

  • Fixes issue where field descriptions could render raw HTML inside .
  • Adds min-width to td/th and max-height to headers in the table rendering to prevent excessively tall table heads when displaying long text.

Testing instructions

  1. Create a query with fields that have raw HTML in their descriptions and very long text strings.
  2. Render the dataquery table.
  3. Verify the HTML tags are safely escaped and not rendered as active HTML.
  4. Verify the table header layout remains constrained (doesn't stretch vertically).

Link to related issue

@github-actions github-actions Bot added Language: Javascript PR or issue that update Javascript code Module: dataquery PR or issue related to (new) dataquery module labels Jul 28, 2026
@HenriRabalais

Copy link
Copy Markdown
Collaborator

@adamdaudrich can you take a look at this since you created the issue?

@adamdaudrich

Copy link
Copy Markdown
Contributor

LGTM
The"real world" usecase was a project in CBIGR where field names came from the following pipeline:
redcap-->redcap2linst-->DQT.
So the problem was how their redcap was setup and how the fields got translated to linst. We used a php library function to strip html out, but still...
if you have cbig repo permission, check instrument 'abasiii_521', the static ones.

@HenriRabalais

Copy link
Copy Markdown
Collaborator

Ok, I'll do a code review in the coming days!

@HenriRabalais HenriRabalais left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @marandmart! took a proper look, mostly small stuff but I'd like to sort a few things before approving:

  1. the stripHTML is going into getDictionaryDescription, which is used in 6 places (fielddisplay, welcome, viewdata, definefields), not just the table header. so this strips html from descriptions everywhere now… is that intentional, or should it be scoped to just the header in viewdata? I could see it being an improvement across the board but wanted to check!
  2. for the label type going to string | ReactNode… does the label actually need to be a ReactNode? could we keep it a string and handle the header sizing on the th directly instead of wrapping the value in a <div>? and if it does need to change, ReactNode already includes string, so the union can just be ReactNode.
  3. the <style> block is injected inline in the render, and there's no css file in the dataquery module yet. could we move it into a proper .css file and import it at the top? (the way Filter.tsx does with Filter.css)
  4. the min-width: 150px is on both th and td, so it widens every column, not just the header, which'll add horizontal scroll on wider result sets. was that intended?

nothing major, just want to make sure we're not stripping html or forcing column widths in more places than we mean to! let me know 🙂

@marandmart

Copy link
Copy Markdown
Contributor Author

Hi @HenriRabalais!

  1. No, not intentional. I missed that that method is being used in other parts of the code. Should I change it to make it apply only to the viewdata part or do you think it makes sense to leave it in that method?
  2. I tried applying the styles directly to the th to limit its’ height but they weren’t working correctly. The workaround I found was to wrap the text content around a div and applying the styles to the div and then the solution worked.
  3. Done!
  4. Yes! Overall I thought it looked slightly better to apply a min-width that makes the columns’ width more consistent with each other. But I guess its more of preference, if you don't think it necessary I can remove that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language: Javascript PR or issue that update Javascript code Module: dataquery PR or issue related to (new) dataquery module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[dataquery] scrolling query

3 participants