-
Notifications
You must be signed in to change notification settings - Fork 0
Modernize docs #97
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
base: dev
Are you sure you want to change the base?
Modernize docs #97
Conversation
Reviewer's GuideModernizes auro-dialog’s documentation and demos, formalizes the public API (including a new lg size and register(name) method), improves typing and event dispatch in the component, and updates tooling/package metadata including custom-elements manifest support. Sequence diagram for bubbling toggle event from auro-dialogsequenceDiagram
actor User
participant AppScript
participant Dialog as auro-dialog
User->>AppScript: Click close UI (e.g. button in footer)
AppScript->>Dialog: Set open = false / remove open attribute
Dialog->>Dialog: dispatchToggleEvent()
Dialog-->>AppScript: toggle event (bubbles=true, cancelable=false)
AppScript->>AppScript: Handle toggle listener on ancestor element
Class diagram for updated auro-dialog component APIclassDiagram
direction LR
class ComponentBase {
+boolean modal
+boolean unformatted
+"default"|"inverse" closeButtonAppearance
+boolean lg
+boolean md
+boolean onDark
+boolean open
+boolean sm
+HTMLElement triggerElement
+dispatchToggleEvent()
}
class AuroDialog {
+static register(name)
+open
}
AuroDialog --|> ComponentBase
State diagram for auro-dialog open and modal behaviorstateDiagram-v2
[*] --> Closed
Closed --> OpenNonModal: set open=true
Closed --> OpenModal: set open=true and modal=true
OpenNonModal --> Closed: user clicks overlay or close button
OpenNonModal --> Closed: script sets open=false
OpenModal --> Closed: footer action closes dialog
OpenModal --> Closed: script sets open=false
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- In
AuroDialog.properties, usingObject.assign(ComponentBase.properties.open, { noAccessor: true })mutates the shared descriptor onComponentBase; consider cloning instead (e.g.{ ...ComponentBase.properties.open, noAccessor: true }) to avoid side effects on other consumers ofComponentBase.properties.open. - The
@aurodesignsystem/auro-clidevDependency now points to a local tarball (file:../../../aurodesignsystem-auro-cli-0.0.0.tgz); if this is not intentional for long-term use, you may want to revert to a published version to keep installs working outside your local environment.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `AuroDialog.properties`, using `Object.assign(ComponentBase.properties.open, { noAccessor: true })` mutates the shared descriptor on `ComponentBase`; consider cloning instead (e.g. `{ ...ComponentBase.properties.open, noAccessor: true }`) to avoid side effects on other consumers of `ComponentBase.properties.open`.
- The `@aurodesignsystem/auro-cli` devDependency now points to a local tarball (`file:../../../aurodesignsystem-auro-cli-0.0.0.tgz`); if this is not intentional for long-term use, you may want to revert to a published version to keep installs working outside your local environment.
## Individual Comments
### Comment 1
<location> `docs/api.md:3` </location>
<code_context>
-| `dialog-footer` | apply CSS to the footer of the dialog |
-| `dialog-header` | apply CSS to the header of the dialog |
-| `dialog-overlay` | apply CSS on the overlay of the dialog |
+The auro-dialog appears above the page and presents information that requires the users immediate attention.
+
+### Properties & Attributes
</code_context>
<issue_to_address>
**issue (typo):** Use the possessive form "user's" in this sentence.
Change this phrase to `requires the user's immediate attention` to use the correct possessive form.
```suggestion
The auro-dialog appears above the page and presents information that requires the user's immediate attention.
```
</issue_to_address>
### Comment 2
<location> `docs/partials/api.md:87` </location>
<code_context>
For use case where the use of a dialog is to be more freeform, but the experience and base tooling for the dialog are still requested, there is the `unformatted` property.
This property can be used in combination of any other use case of the dialog, but it will render a unformatted dialog window allowing for full customization of content within the scope of the window.
-### Responsive padding
</code_context>
<issue_to_address>
**suggestion (typo):** Improve grammar in this sentence ("combination with" and "an unformatted").
Suggested revision: `This property can be used in combination with any other use case of the dialog, but it will render an unformatted dialog window, allowing for full customization of content within the scope of the window.`
```suggestion
This property can be used in combination with any other use case of the dialog, but it will render an unformatted dialog window, allowing for full customization of content within the scope of the window.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
69eb3d5 to
6f19ea0
Compare
|
🚀 PR Release Published! To install: npm install @aurodesignsystem-dev/[email protected]Install via alias: npm install @aurodesignsystem/auro-dialog@npm:@aurodesignsystem-dev/[email protected] |
Component Demo Linksdialog: https://auro-dialog-97.surge.sh |
6f19ea0 to
0c2a8e9
Compare
0c2a8e9 to
5591f50
Compare
5591f50 to
ae890f6
Compare
ae890f6 to
9fa51b3
Compare
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.
Hey - I've found 2 issues, and left some high level feedback:
- In package.json, the entry "./custom-elements.json": "./custom-elements.json" under "scripts" looks accidental/misplaced (you already expose this via exports and customElements) and should likely be removed or renamed to a valid script name.
- In docs/api.md, the attribute name for the onDark/on-dark flag appears inconsistent with the implementation (the reflected attribute will be
on-dark, notonDark), so the API table should be updated to use the correct kebab-case attribute name.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In package.json, the entry "./custom-elements.json": "./custom-elements.json" under "scripts" looks accidental/misplaced (you already expose this via exports and customElements) and should likely be removed or renamed to a valid script name.
- In docs/api.md, the attribute name for the onDark/on-dark flag appears inconsistent with the implementation (the reflected attribute will be `on-dark`, not `onDark`), so the API table should be updated to use the correct kebab-case attribute name.
## Individual Comments
### Comment 1
<location> `package.json:56` </location>
<code_context>
"scripts": {
"build": "auro build",
"build:version": "node ./scripts/version.mjs",
+ "./custom-elements.json": "./custom-elements.json",
"dev": "auro dev -w -s",
"dev:open": "auro dev -w -s --open",
</code_context>
<issue_to_address>
**issue (bug_risk):** The new scripts entry for `./custom-elements.json` looks malformed for an npm script.
`scripts` keys should be script names mapped to commands (e.g. `
</issue_to_address>
### Comment 2
<location> `src/auro-dialog.js:19` </location>
<code_context>
- ...ComponentBase.properties.open,
- noAccessor: true,
- },
+ open: Object.assign(ComponentBase.properties.open, { noAccessor: true }),
};
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `Object.assign` here mutates the base `ComponentBase.properties.open` descriptor, which may have unintended side effects.
Since `Object.assign` mutates its first argument, this line permanently adds `noAccessor: true` to `ComponentBase.properties.open` for all subclasses. That globally changes the base behavior. Instead, keep the base descriptor immutable by creating a shallow copy, e.g. `open: { ...ComponentBase.properties.open, noAccessor: true }`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
9fa51b3 to
9b58ad8
Compare
example code updated to reflect latest Auro element APIs example documentation layout improved custom registration updated and moved to install page core element descriptions updated properties now listed alphabetically doc build system now uses the latest version of auro-cli package json configuration updated to support latest auro-cli additional packages dependencies updated to latest versions
9b58ad8 to
035521f
Compare
Alaska Airlines Pull Request
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
PR Reviewer Checklist:
Documentation Accuracy
Stringproperties displaying all of their "options"?indexpage (i.e. only the most "basic" example(s) of component)installpage and renders as expectedCode Changes
myExample.html->my-example.html)perfcommit to trigger a release, as expected?Visual Checks
See Codeaccordions actually render the code blocks under their respective examplesSummary by Sourcery
Modernize auro-dialog documentation, clarify API usage, and align the component implementation and tooling with the updated docs.
New Features:
lgsize attribute/property support to auro-dialog for decoupled desktop and mobile sizing.register(name)method on auro-dialog for custom element registration and version management.Enhancements:
closeButtonAppearance.cemnpm script andcustomElementspackage.json field.Documentation:
registerAPI.Summary by Sourcery
Modernize the auro-dialog component documentation and supporting tooling while clarifying size, modal, and registration behavior.
Enhancements:
Build:
Documentation:
Chores: