Skip to content

Conversation

snowystinger
Copy link
Member

@snowystinger snowystinger commented Sep 17, 2022

Closes

One thing to note, on overflow wrap tables, the headers have now gotten taller by 4px because we start with an estimated height and then calculate after render the actual height.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@adobe-bot
Copy link

reidbarber
reidbarber previously approved these changes Sep 19, 2022
Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

LGTM

@devongovett
Copy link
Member

Looking at https://spectrum.adobe.com/page/table/, I think the font size looks too big, and the color looks too dim.

@adobe-bot
Copy link

@adobe-bot
Copy link

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

Partial review, need to track down the other component XD files.

@@ -16,19 +16,20 @@ governing permissions and limitations under the License.
}

.spectrum-Table-headCell {
color: var(--spectrum-table-header-text-color);
color: var(--spectrum-alias-text-color);
Copy link
Member

Choose a reason for hiding this comment

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

The text color in the XD file is #000000/rgb(0,0,0), but this var resolves to rgb(75,75,75) on light. Do we need to adjust the value of the var? Or is it just that we need to update our spectrum vars?

Copy link
Member Author

Choose a reason for hiding this comment

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

update our spectrum vars
for any reviewers who don't already know, you can get the spectrum plugin for XD, then when you click on a component, it'll tell you the spectrum variable names. you can swap between version 5 and 6

Copy link
Member

Choose a reason for hiding this comment

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

Is this the same issue I'm seeing where the sort direction indicator looks darker in the XD file and v5 colors says it should be #6d6d6, closest Spectrum being gray-700, but we're using #8e8e8e/rgb(142,142,142). When I look at the spectrum token of that color we're using gray-600, which isn't gray-700.

@adobe-bot
Copy link

@adobe-bot
Copy link

devongovett
devongovett previously approved these changes Sep 22, 2022
@ktabors
Copy link
Member

ktabors commented Sep 22, 2022

Where are the accordion XD files? I'm sure the larger and darker side is desired it just looks weird. I wanted to check the chevron because the open state seems too low. Probably not a priority since this is alpha and main work is being done by Quarry.

@@ -16,19 +16,20 @@ governing permissions and limitations under the License.
}

.spectrum-Table-headCell {
color: var(--spectrum-table-header-text-color);
color: var(--spectrum-alias-text-color);
Copy link
Member

Choose a reason for hiding this comment

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

Is this the same issue I'm seeing where the sort direction indicator looks darker in the XD file and v5 colors says it should be #6d6d6, closest Spectrum being gray-700, but we're using #8e8e8e/rgb(142,142,142). When I look at the spectrum token of that color we're using gray-600, which isn't gray-700.

@snowystinger
Copy link
Member Author

snowystinger commented Sep 22, 2022

Where are the accordion XD files? I'm sure the larger and darker side is desired it just looks weird. I wanted to check the chevron because the open state seems too low. Probably not a priority since this is alpha and main work is being done by Quarry.

accordion xd are in the contributions site

Is this the same issue I'm seeing where the sort direction indicator looks darker in the XD file and v5 colors says it should be #6d6d6, closest Spectrum being gray-700, but we're using #8e8e8e/rgb(142,142,142). When I look at the spectrum token of that color we're using gray-600, which isn't gray-700.

Sort icon was wrong, should be gray-800 like the text, on hover they become gray-900

@adobe-bot
Copy link

@adobe-bot
Copy link

@snowystinger snowystinger merged commit 08cb92a into main Sep 23, 2022
@snowystinger snowystinger deleted the do-not-uppercase-table-headers branch September 23, 2022 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants