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

Initial draft of cem-* elements for rendering manifest data #1425

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kevinpschaaf
Copy link
Collaborator

No description provided.

@github-actions
Copy link

A live preview of this PR will be available at the URL below and will update on each commit. =
The build takes ~5-10 minutes, and will 404 until finished.

https://pr1425---site-khswqo4xea-wl.a.run.app/

Warning
Until our Cloud Run project is public, only authorized users can view the above URL.
The easiest way to view it authenticated is to run the following proxy command and visit http://localhost:5453

(gcloud beta run services proxy --project=webcomponents-org-test --region=us-west2 --tag=pr1425 --port=5453 site \
& gcloud beta run services proxy --project=webcomponents-org-test --region=us-west2 --tag=pr1425 --port=6453 catalog)

Copy link
Collaborator

@justinfagnani justinfagnani left a comment

Choose a reason for hiding this comment

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

This seems ok to me to submit as a first cut!

</div>
<cem-class-declaration id="content" .declaration=${declaration}>
<div slot="usage">
<h4>Usage</h4>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the title go in the element so it's standardized?

"lit": "^2.6.0",
"lit-analyzer": "^1.2.1"
"lit-analyzer": "^1.2.1",
"marked": "^4.2.5"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Todo: I think we should make a separate package that has a markdown parser pre-configured with all the plugins we accept so we make sure it's consistent across uses.

@@ -0,0 +1,292 @@
/**
* @license
* Copyright 2022 Google LLC
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Copyright 2022 Google LLC
* Copyright 2023 Google LLC

>deprecated
</span>`
)}
${privacy !== undefined && privacy !== 'public'
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe pull up into a named variable so it can have a comment, like:

// only display private and protected privacy
const privacyString = privacy !== ...

: ''}${statik ? 'static ' : ''}${rest ? '...' : ''}
${name}</code
>
${optional ? html`<span class="optional"> (optional)</span>` : ''}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm assuming there's markup here and not for other modifiers because of the drafty nature of the PR? If so, I think I'd include a span/class for all modifiers or none and we can add them as needed later?

parameters,
(params: cem.Parameter[]) =>
html`
<table>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might want to consider using CSS grid so that multiple parameters tables have their columns line up.

@customElement('cem-class-declaration')
export class CemClassDeclaration extends LitElement {
static styles = styles;
@property()
Copy link
Collaborator

Choose a reason for hiding this comment

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

formatting nit: give these some newlines so they have rom to breathe

this.declaration.members?.filter(
(m) => m.kind === 'field' && m.privacy !== 'private'
),
(m) => html` <h4>Fields</h4>
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Prettier maintains leading whitespace if you initially had a newline. You can search for `` ` to find and remove these.

@@ -0,0 +1,94 @@
/**
* @license
* Copyright 2022 Google LLC
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Copyright 2022 Google LLC
* Copyright 2023 Google LLC

here and elsewhere

export const styles = css`
:host {
display: block;
padding: 0 0 15px 15px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

we'll need variables for all of these spacing and colors.

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.

2 participants