-
Notifications
You must be signed in to change notification settings - Fork 2
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
USWDS-proposals: Add directory for pre-alpha web components #18
base: main
Are you sure you want to change the base?
Conversation
adding links and naming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the edits I made, looks good to me. But check my work too, y'all. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple potential suggestions and questions! Nothing blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Does the rest of the proposals file structure make sense with this change? Should we create a core dir as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good note. The repo was originally designed to just be for component proposals, and I think the recent repo growth is showing strangely in both the file structure and base README. Now that the repo will also include ADRs and Elements pre-alphas, we should at least address that in the README and consider if the file structure still makes sense. I've opened #19 to address the README piece of this.
web-components/README.md
Outdated
|
||
## How to create a pre-alpha document | ||
|
||
1. Create a branch for the web component pre-alpha using the following naming structure: `[component-name]-pre-alpha` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Should we prefix the PRs with our initials?
1. Create a branch for the web component pre-alpha using the following naming structure: `[component-name]-pre-alpha` | |
1. Create a branch for the web component pre-alpha using the following naming structure: `[initials]-[component-name]-pre-alpha` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point @mahoneycm. Do you see any issues with not having initials?
We do this with code to avoid collision in branch names when we're working on the same feature.
It's doubtful that two of us would be working on the same pre-alpha document. Although it can happen if we want to make significant changes via a PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the recommended branch structure to [contributor initials]-pre-alpha-[component-name]
in 2e1e23f.
Let me know if you have any objections or concerns.
web-components/_template.md
Outdated
You can find these files in Google Drive: | ||
https://drive.google.com/drive/folders/1F8z7v5RCOV3D33iEz7TR0B8IHv20WNMF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Should we make a note that this link is internal only?
I know we have the note that only the core team can make the proposals for now but I'm curious if we should make note in case anyone is poking around the files.
Might not be important!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a "limited access" note in 5020a85.
web-components/_template.md
Outdated
|
||
<!-- | ||
| USWDS 3 variant | Web components variant | Description | Defined via | | ||
|--------|--------|--------|--------| | ||
| | | | | | ||
--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Not sure I follow what the the Web components variant
is supposed to be listed as here.
Is this just the name of the variant? Would this differ from the USWDS 3 variant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for creating this @amyleadem. Overall it looks good. Left a few comments, but nothing is blocking.
I've also made some formatting changes using the built-in markdown formatter in Prettier.
web-components/README.md
Outdated
|
||
## How to create a pre-alpha document | ||
|
||
1. Create a branch for the web component pre-alpha using the following naming structure: `[component-name]-pre-alpha` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: Flipping this could cleanly separate the pre-alpha branches listed from others.
- usa-modal-pre-alpha
- usa-accordion-pre-alpha
- usa-text-input-pre-alpha
+ pre-alpha-usa-modal
+ pre-alpha-usa-accordion
+ pre-alpha-usa-text-input
Alternatively, we could even take an approach like the web components repo and do something like:
pre-alpha/usa-accordion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the recommended branch structure to [contributor initials]-pre-alpha-[component-name]
in 2e1e23f.
Let me know if you have any objections or concerns.
web-components/_template.md
Outdated
_Provide a link to the code samples from the landscape analysis._ | ||
|
||
<!-- | ||
You can find these files in Google Drive: | ||
https://drive.google.com/drive/folders/1F8z7v5RCOV3D33iEz7TR0B8IHv20WNMF | ||
--> | ||
|
||
[Component] code samples from the landscape analysis (Google docs :lock:) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Confirming, we're only linking to the code samples and not the landscape analysis spreadsheet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I don't think we need to do a full analysis of each components. It feels more appropriate to just grab code samples for comparison.
web-components/_template.md
Outdated
#### Current variants | ||
|
||
<!-- | ||
| USWDS 3 variant | Web components variant | Description | Defined via | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: What does Defined via
mean? What are the options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was intended to capture how the user would set this variant in the light DOM.
Since I imagine this would almost always be a prop of some sort, I updated the heading to "Related prop" in 5020a85.
Let me know if that is still unclear.
web-components/README.md
Outdated
|
||
## How to create a pre-alpha document | ||
|
||
1. Create a branch for the web component pre-alpha using the following naming structure: `[component-name]-pre-alpha` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point @mahoneycm. Do you see any issues with not having initials?
We do this with code to avoid collision in branch names when we're working on the same feature.
It's doubtful that two of us would be working on the same pre-alpha document. Although it can happen if we want to make significant changes via a PR.
web-components/_template.md
Outdated
|
||
### Critical content | ||
|
||
_In a table, list the component content that is critical for user understanding. These elements should always be painted on the page, even when JavaScript is unavailable or slow._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
polish: Small suggestion to remove the word painted
because it might be too technical.
_In a table, list the component content that is critical for user understanding. These elements should always be painted on the page, even when JavaScript is unavailable or slow._ | |
_In a table, list the component content that is critical for user understanding. These elements should always be on the page, even when JavaScript is unavailable or slow._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 5020a85
web-components/_template.md
Outdated
|
||
### Light DOM | ||
|
||
_Provide a code sample for the web component in the light DOM. Provide default and complex implementations when applicable._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
polish: The heading already mentions light DOM. We can use plain language for the requirements.
_Provide a code sample for the web component in the light DOM. Provide default and complex implementations when applicable._ | |
_Provide an example for how the web component would be used. Include a default example and complex example (when applicable)._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the copy to "Provide a code sample of how a user would add this web component to their project. Provide a default example and a complex example (when applicable)." in 5020a85
_Provide a code sample of how the component will be built in the shadow DOM. Provide default and complex implementations when applicable._ | ||
|
||
<!-- | ||
```html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
polish: We could add clarity by adding an HTML comment marking that this is the shadow DOM markup.
```html | |
```html | |
<!-- #shadow-root --> | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Added in 5020a85
Summary
Create a README and template for the web component pre-alphas.
Note
The pre-alpha requirements are documented in the Web Components release lifecycle doc (Google docs 🔒)
Preview links
Testing and review