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(alert): add pf-alert #2593

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

feat(alert): add pf-alert #2593

wants to merge 15 commits into from

Conversation

brianferry
Copy link
Collaborator

#2539

What I did

Testing Instructions

Notes to Reviewers

@changeset-bot
Copy link

changeset-bot bot commented Sep 19, 2023

🦋 Changeset detected

Latest commit: 472fa59

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@patternfly/pfe-core Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added work in progress POC / Not ready for review demo Updating demo pages functionality Functionality, typically pertaining to the JavaScript. styles An issue or PR pertaining only to CSS/Sass tests Related to testing doc labels Sep 19, 2023
@brianferry brianferry linked an issue Sep 19, 2023 that may be closed by this pull request
@netlify
Copy link

netlify bot commented Sep 19, 2023

Deploy Preview for patternfly-elements ready!

Name Link
🔨 Latest commit 91a5b64
😎 Deploy Preview https://deploy-preview-2593--patternfly-elements.netlify.app/

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions bot added the AT passed Automated testing has passed label Sep 19, 2023
@bennypowers bennypowers changed the title feat: add pf-alert feat(alert): add pf-alert Sep 27, 2023
Copy link
Member

@bennypowers bennypowers left a comment

Choose a reason for hiding this comment

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

some preliminary comments. i didn't get to the lifecycle or template or styles yet

elements/pf-alert/demo/pf-alert.html Outdated Show resolved Hide resolved
core/pfe-core/controllers/slot-controller.ts Outdated Show resolved Hide resolved
elements/pf-alert/README.md Outdated Show resolved Hide resolved
elements/pf-alert/pf-alert.css Outdated Show resolved Hide resolved
elements/pf-alert/pf-alert.ts Show resolved Hide resolved
elements/pf-alert/pf-alert.ts Outdated Show resolved Hide resolved
elements/pf-alert/pf-alert.ts Outdated Show resolved Hide resolved
elements/pf-alert/pf-alert.ts Outdated Show resolved Hide resolved
elements/pf-alert/pf-alert.ts Show resolved Hide resolved
elements/pf-alert/pf-alert.ts Outdated Show resolved Hide resolved
Copy link
Member

@bennypowers bennypowers left a comment

Choose a reason for hiding this comment

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

some more notes

elements/pf-alert/pf-alert.ts Outdated Show resolved Hide resolved
elements/pf-alert/pf-alert.ts Outdated Show resolved Hide resolved
elements/pf-alert/pf-alert.ts Outdated Show resolved Hide resolved
elements/pf-alert/pf-alert.ts Outdated Show resolved Hide resolved
const hasContent = this.#slots.hasAnonymousSlot();

return html`
<div id="container" role="alert" aria-hidden="false" class="${classMap({ truncateTitle })}">
Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines 1 to 2
<link rel="stylesheet" href="demo.css">
<script type="module" src="pf-alert.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

let's prefer to inline this stuff, to make the demos easier to grok

Comment on lines 1 to 2
<link rel="stylesheet" href="demo.css">
<script type="module" src="pf-alert.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

see above, re: inline

Comment on lines 4 to 7
<h1>
Inline
</h1>

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<h1>
Inline
</h1>

Comment on lines 14 to 17
<h1>
Inline Plain
</h1>

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<h1>
Inline Plain
</h1>
<h2>Plain</h2>

Comment on lines 1 to 2
<link rel="stylesheet" href="demo.css">
<script type="module" src="pf-alert.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

inline

Comment on lines 5 to 8
<pf-alert variant="info" header="Info alert title"></pf-alert>
<pf-alert variant="success" header="Success alert title"></pf-alert>
<pf-alert variant="warning" header="Warning alert title"></pf-alert>
<pf-alert variant="danger" header="Danger alert title"></pf-alert>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<pf-alert variant="info" header="Info alert title"></pf-alert>
<pf-alert variant="success" header="Success alert title"></pf-alert>
<pf-alert variant="warning" header="Warning alert title"></pf-alert>
<pf-alert variant="danger" header="Danger alert title"></pf-alert>

we can leave these for variants.html

Comment on lines 1 to 2
<link rel="stylesheet" href="demo.css">
<script type="module" src="pf-alert.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

inline

--pf-alert-title-color: var(--pf-alert-title-color, var(--pf-global--default-color--300, #003737));
--pf-alert-icon-color: var(--pf-alert-icon-color, var(--pf-global--default-color--200, #009596));
--pf-alert-inline-background-color: var(--pf-global--palette--cyan-50, #f2f9f9);
--alert-title-color: var(--alert-title-color, var(--pf-global--default-color--300, #003737));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
--alert-title-color: var(--alert-title-color, var(--pf-global--default-color--300, #003737));
--_alert-title-color: var(--pf-global--default-color--300, #003737);

etc, and at call sites

danger: { set: 'fas', icon: 'exclamation-circle' },
info: { set: 'fas', icon: 'info-circle' },
close: { set: 'patternfly', icon: 'close' },
get(name: 'default' | 'success' | 'warning' | 'danger' | 'info' | 'close') {
Copy link
Member

Choose a reason for hiding this comment

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

remove get() and reference the data here in render

const hasActions = this.#slots.hasSlotted('actions');
const hasDescriptionContent = this.#slots.hasSlotted();

return html`
<div id="container" role="alert" aria-hidden="false" class="${classMap({ truncateTitle })}">
<div id="left-column" part="left-column">
<slot name="icon" id="icon">${icon}</slot>
<slot name="icon" id="icon">${this.#icon}</slot>
Copy link
Member

Choose a reason for hiding this comment

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

inline the pf-icon, reference data in ICONS directly here. remove #icon

@@ -1,5 +1,4 @@
<link rel="stylesheet" href="demo.css">
<script type="module" src="pf-alert.js"></script>
<link rel="stylesheet" href="demo.css"><script type="module" src="pf-alert.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

For the sake of clarity, What I mean by inline is to replace <link href with <style and <script src with <script

In other words, each demo file would be self contained except for the files we ship in our package

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AT passed Automated testing has passed demo Updating demo pages doc functionality Functionality, typically pertaining to the JavaScript. styles An issue or PR pertaining only to CSS/Sass tests Related to testing work in progress POC / Not ready for review
Projects
Status: Ready: code & design review
Development

Successfully merging this pull request may close these issues.

[1:1]: pf-alert
2 participants