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

docs(ui-table): function based examples are added to Table #1663

Merged

Conversation

Kadirsaglm
Copy link
Contributor

No description provided.

Comment on lines -334 to -336
<Table.Cell
key={headerId}
width={headers.find(header => header.id === headerId).width}
Copy link
Contributor

Choose a reason for hiding this comment

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

This width props was deleted from the original class based example and missing from the functional too. Is there a reason for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the docs Table.Cell doesn't have width prop and also in console below warning is shown

console.js:53 Warning: [Cell] prop 'width' is not allowed.

That's why I removed the width prop. If it is needed for some kind of a workaround solution I can add it back.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, thanks for the clarification

<FormField id="columnTextAlign" label="Set column text-align">
<Flex margin="0 0 medium">
{Object.entries(colTextAligns).map(([headerId, textAlign]) => {
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole Formfield is missing from the function-based component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just realized that the renderOptions function in the class-based example has two return statements, with the second one including the FormField. I think we should remove the unreachable return statement completely from the class-based component.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, it should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines +712 to 683
- ```javascript
const SortableTable = ({ caption, headers, rows }) => {
const initialColWidth = {}
headers.forEach((header) => {
initialColWidth[header.id] = 'start'
})
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole functional example doesn't work, it throws an error: TypeError: Cannot read properties of undefined (reading 'onRequestSort'). Can you please look into this again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't reproduce this issue in my local. I can sort the table as expected. Could you provide more context like if there is any like special settings set etc.?

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked it again, it works now 👍

<Table.Row>
{(headers || []).map(({ id, text, width }) => (
{headers.map(({ id, text, width }) => (
Copy link
Contributor

@ToMESSKa ToMESSKa Sep 3, 2024

Choose a reason for hiding this comment

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

Here there is truthy check in the class-based example "((headers || []).map(...", which is missing from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return (
<Tag
style={rowStyle}
role={isStacked ? 'row' : undefined}
Copy link
Contributor

Choose a reason for hiding this comment

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

This role prop is missing from the functional example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

.map((child, index) => {
return React.cloneElement(child, {
key: child.props.name,
// used by `CustomTableCell` to render its column title in `stacked` layout
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment should be in the functional example too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@Kadirsaglm Kadirsaglm force-pushed the write_functional_examples_for_table branch from 363ba97 to b9e3fd3 Compare September 3, 2024 12:36
@Kadirsaglm Kadirsaglm force-pushed the write_functional_examples_for_table branch from b9e3fd3 to 5d16abe Compare September 4, 2024 18:53
@ToMESSKa ToMESSKa merged commit cfa9dc6 into instructure:master Sep 10, 2024
10 of 11 checks passed
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.

3 participants