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

DataViews Field rendering: Context should be not be assumed by fields when rendering #56320

Open
andrewhayward opened this issue Nov 20, 2023 · 1 comment
Labels
[Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Enhancement A suggestion for improvement.

Comments

@andrewhayward
Copy link
Contributor

andrewhayward commented Nov 20, 2023

Related: #55083

What problem does this address?

Field rendering is context-unaware, and the same output is generated regardless of which layout is being used. This means that we should ensure rendered content is appropriate in any view.

For example, while <h3>{ title }</h3> is appropriate in a grid, it should be <th scope="row">{ title }</th> in a list.

The Data Views grid layout, with an object title circled in red The Data Views list layout, with an object title circled in red

What is your proposed solution?

Layouts should render their own context (e.g. <h3>, <th>) and fields should not be opinionated on this. Currently, for example, the list view renders its own <td>s, which wrap every field:

{ row.getVisibleCells().map( ( cell ) => (
<td
key={ cell.id }
style={ {
width:
cell.column.columnDef.width ||
undefined,
maxWidth:
cell.column.columnDef
.maxWidth || undefined,
} }
>
{ flexRender(
cell.column.columnDef.cell,
cell.getContext()
) }
</td>
) ) }
</tr>

The DataViews component could gain a primaryField prop, or the fields API could be adjusted to include an isPrimary flag, either of which layouts could use to render context appropriately...

<DataViews
  ...
  fields={ fields }
  primaryField="title"
  ...
/>
const fields = [
  ...
  {
	id: 'title',
	...
	isPrimary: true,
	...
  },
  ...
];

The alternative is to make sure that field.render checks the view type that is passed in, but that adds complexity that is probably not worth it. Every field would have to know to render a <th> or <td> in some instances, an <h3> in others, and as the list of available layouts changes, they would have to stay on top of that.

@andrewhayward andrewhayward added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond labels Nov 20, 2023
@oandregal
Copy link
Member

I'm exploring a potential path at #56942

@oandregal oandregal changed the title [Data Views] Field rendering: Context should be not be assumed by fields when rendering DataViews Field rendering: Context should be not be assumed by fields when rendering Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

2 participants