Skip to content

Conversation

kseamon
Copy link
Collaborator

@kseamon kseamon commented Dec 27, 2023

  1. Uplevels repeated logic to the host CdkTable which does it once instead of n times.
  2. Skips setting role=cell in native tables, for which it is already the default (see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/td#technical_summary)

This saves just over a tenth of a millisecond per cell in the native TD case and about a twentieth in other cases. Doesn't sound like much, but it adds up in larger tables.

@kseamon kseamon added the G This is is related to a Google internal issue label Dec 27, 2023
@crisbeto
Copy link
Member

crisbeto commented Jan 2, 2024

@kseamon can you run yarn approve-api cdk/table and commit the result?

@kseamon kseamon force-pushed the table-cell-micro-optimize-more branch from 22b0d4b to c62d561 Compare January 2, 2024 16:15
@kseamon
Copy link
Collaborator Author

kseamon commented Jan 2, 2024

@kseamon can you run yarn approve-api cdk/table and commit the result?

Done!

@kseamon kseamon force-pushed the table-cell-micro-optimize-more branch from c62d561 to d4fb042 Compare January 4, 2024 19:47
@kseamon
Copy link
Collaborator Author

kseamon commented Jan 4, 2024

Rejiggered a bit to account for a few internal cases where the table's role was changing between its constructor and the first cell's constructor.

@kseamon kseamon force-pushed the table-cell-micro-optimize-more branch from 3528c80 to de794e4 Compare January 5, 2024 21:20
@crisbeto
Copy link
Member

crisbeto commented Jan 8, 2024

@kseamon looks like this needs a rebase and there's a lint failure.

@kseamon kseamon force-pushed the table-cell-micro-optimize-more branch from de794e4 to edab25b Compare January 8, 2024 18:02
@kseamon
Copy link
Collaborator Author

kseamon commented Jan 8, 2024

@kseamon looks like this needs a rebase and there's a lint failure.

Rebased. The internal CL for this (596613799) is already submitted. Is it possible to handle the lint issue in a fast follow PR?

@andrewseguin
Copy link
Contributor

@kseamon Yup no problem, we can merge this and fix the lint issue with an immediate followup PR

@andrewseguin andrewseguin added target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker labels Jan 8, 2024
@andrewseguin andrewseguin added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Jan 8, 2024
@andrewseguin andrewseguin merged commit 1fe1f69 into angular:main Jan 8, 2024
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker G This is is related to a Google internal issue target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants