[webkit-reviews] review denied: [Bug 214563] Web Inspector: Change DataGrid and Table styles to closer match macOS : [Attachment 404807] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Nikita Vasilyev <nvasilyev at apple.com>'s
request for review:
Bug 214563: Web Inspector: Change DataGrid and Table styles to closer match
macOS
https://bugs.webkit.org/show_bug.cgi?id=214563

Attachment 404807: Patch

https://bugs.webkit.org/attachment.cgi?id=404807&action=review




--- Comment #8 from Brian Burg <bburg at apple.com> ---
Comment on attachment 404807
  --> https://bugs.webkit.org/attachment.cgi?id=404807
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404807&action=review

You haven't commented on which of these changes is appropriate for Big Sur
only.

> Source/WebInspectorUI/ChangeLog:22
> +	   In macOS Catalina and Big Sur, sorted table header has bold text but
the background doesn't change.

Table headers are not bolded on Catalina. Please make this for Big Sur only.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:-91
> -body[dir=ltr] .data-grid :matches(th, td):not(:last-child) {

Please explain why we've changed from styling the <th> to styling the inner
div. I don't understand why the change is needed.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:95
> +    --horizontal-padding: 6px;

Why is this variable defined with so much specificity? It looks wrong to define
it here.

Please rename to --data-grid-header-horizontal-padding or something. It needs
to be prefixed with --data-grid.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:111
> +    font-weight: 600;

This should be a datagrid-specific variable.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:233
> +    -webkit-padding-end: calc(12px + var(--horizontal-padding));

Please add a comment that this extra 12px is for the sorting chevron.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:382
> +body[dir=ltr] .data-grid th:first-child > div {

Why is matching 'td' no longer needed?

> Source/WebInspectorUI/UserInterface/Views/Table.css:156
> +    --vertical-margin: 4px;

Please prefix with --data-grid-. Please put its declaration before its uses for
ease of reading the code.