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

[BD-46] feat: update DataTable font size to be small #1009

Merged

Conversation

monteri
Copy link
Contributor

@monteri monteri commented Jan 5, 2022

Font size equals to $font-size-sm for all elements in the table.

JIRA: PAR-609

@monteri monteri requested a review from a team as a code owner January 5, 2022 12:14
@openedx-webhooks openedx-webhooks added blended PR is managed through 2U's blended developmnt program community manager review labels Jan 5, 2022
@openedx-webhooks
Copy link

Thanks for the pull request, @monteri! I've created BLENDED-1062 to keep track of it in Jira. More details are on the BD-46 project page.

When this pull request is ready, tag your edX technical lead.

@@ -22,6 +22,10 @@ $data-table-pagination-dropdown-min-width: 6rem !default;
border-bottom-left-radius: $border-radius;
border-bottom-right-radius: $border-radius;
}

* {
font-size: $font-size-sm;
Copy link
Member

Choose a reason for hiding this comment

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

@monteri Looking over the Figma spec for DataTable, it seems we may want to be a bit more selective in what we set as $font-size-sm.

Not all the content within DataTable is set as small, but its more focused around the content of the table that should have $font-size-sm, not everything. For example, the font sizes within the filters and actions stay normal size, but the "Showing 1 - 50..." both above and below the table contents and the table contents themselves should have $font-size-sm.

I'd probably just crosscheck with the Figma spec on which parts of DataTable should be small (i.e., by clicking on an element in Figma, you can see what should be small in the sidebar, see screenshot).

image

Should probably have been more specific in the acceptance criteria ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks for clarification, I thought the ticket was meant to make whole text with the small text size :)
Now I placed font-size: $font-size-sm; only for the wrapper so it made the expected result as in the Figma

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thanks for the update!

@monteri monteri force-pushed the zadorozhnii/datatable-fontsize branch from bc28bc7 to 6965fc9 Compare January 6, 2022 15:06
@monteri monteri force-pushed the zadorozhnii/datatable-fontsize branch from 6965fc9 to 8dbfeaf Compare January 6, 2022 15:29
@adamstankiewicz adamstankiewicz merged commit a7f487f into openedx:master Jan 6, 2022
@openedx-webhooks
Copy link

@monteri 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-semantic-release
Copy link
Contributor

🎉 This PR is included in version 17.3.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blended PR is managed through 2U's blended developmnt program merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants