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

feat: add worksheet docs #562

Merged
merged 3 commits into from
Jul 19, 2021

Conversation

jorgemoya
Copy link
Contributor

What?

Adds Worksheet documentation

Screenshots/Screen Recordings

worksheet

@jorgemoya jorgemoya requested review from a team as code owners July 15, 2021 20:54
@jorgemoya jorgemoya force-pushed the worksheet-documentation branch from e0fb0c8 to bf91077 Compare July 15, 2021 20:57
Copy link
Contributor

@chanceaclark chanceaclark left a comment

Choose a reason for hiding this comment

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

Small comments, but looks great!

name: 'hash',
types: 'string',
required: true,
description: 'Unique identifier for column.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we describe this prop a bit more? What are it's usages et. al.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is only used internally, do we need to tell them it is used to manipulate the cells? I copied this from Table btw.

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 guess it is also used on the rows, so that there is some structure present?

Copy link
Contributor

Choose a reason for hiding this comment

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

My main concern is that it's a required prop, without really explaining why the consumer needs to use it. I'm trying to approach this from a perspective of a consumer that doesn't really know the internal implementation here; they just need it to work, but understand a little why.

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 don't know if I agree with this, we rarely (if ever) indicate why an id is required. It can be inferred the component uses it internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated for some more info, let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well for instance, we have the Dropdown component which it's items have a hash prop. The components implementation doesn't use hash at all (if you search the repo) and the description of the prop is Stored hash of the item.. I know that is specific to the Dropdown, but as a consumer, it doesn't give me any knowledge of what it's used for. For the worksheet I want to avoid stuff like this, in case we drop support for hash or etc.. It should be clear what the intended purpose is.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about this?

Unique identifier for the column value of each row. Used internally to identify and manage state values.

name: 'header',
types: 'string',
required: true,
description: 'Header title.',
Copy link
Contributor

Choose a reason for hiding this comment

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

🍹 Maybe make this a little bit more descriptive? Maybe you can think of something better?

Suggested change
description: 'Header title.',
description: 'Header for the column.',

},
{
name: 'type',
types: ['text'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I would check some of these types key/values to see if you can just have them as a string instead of an array.

Suggested change
types: ['text'],
types: 'text',

@jorgemoya jorgemoya force-pushed the worksheet-documentation branch from 8119b87 to def79a1 Compare July 19, 2021 15:52
@jorgemoya jorgemoya merged commit 936a26a into bigcommerce:master Jul 19, 2021
@jorgemoya jorgemoya deleted the worksheet-documentation branch July 19, 2021 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants