Skip to content

Conversation

@ryanseddon
Copy link
Contributor

Adds initial files for containers repo

Borrows heavily from react-components.

@ryanseddon ryanseddon requested a review from jzempel November 23, 2018 05:34
Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

I think this initial setup kind of needs to be gone through with a fine-tooth comb and further updated with content that is contextually relevant for react-containers (i.e. no CSS, Views, Elements, etc.)

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

Getting close @ryanseddon!

"jest": true,
"BASE_PATH_NAME": true,
"PACKAGE_VERSION": true,
"COMPONENT_IDS": true
Copy link
Member

Choose a reason for hiding this comment

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

May not need this as containers don't expose component IDs.

Copy link
Member

Choose a reason for hiding this comment

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

@Austin94 plz weigh-in here, based on recent conversation.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jzempel and I were discussing metrics this morning and came up with an interesting idea.

Would it be helpful to have a new data attribute specifically for the containers? i.e. data-garden-container-id and data-garden-version

This could help us identify areas where consumers have "ejected" into a container and help us identify new patterns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep I think that makes sense

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

Left a comment you may intend to resolve in future PRs on this repo.

@ryanseddon ryanseddon merged commit 179759b into master Dec 11, 2018
@ryanseddon ryanseddon deleted the ryan/initial_setup branch December 11, 2018 23:50
Mikaelia pushed a commit that referenced this pull request Dec 14, 2020
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.

4 participants