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

PR - AppNav Doc #300

Open
wants to merge 6 commits into
base: website
Choose a base branch
from
Open

PR - AppNav Doc #300

wants to merge 6 commits into from

Conversation

laurenic0l
Copy link
Contributor

Here's a first draft of the AppNav doc! Please let me know any changes that need to be made or anything I missed.

closes #261

docs/components/appnav.md Outdated Show resolved Hide resolved
docs/components/appnav.md Outdated Show resolved Hide resolved
docs/components/appnav.md Outdated Show resolved Hide resolved
docs/components/appnav.md Outdated Show resolved Hide resolved
docs/components/appnav.md Outdated Show resolved Hide resolved
docs/components/appnav.md Outdated Show resolved Hide resolved
docs/components/appnav.md Outdated Show resolved Hide resolved

These are the various parts of the [shadow DOM](../glossary#shadow-dom) for the `AppNavItem` component, which will be required when styling via CSS is desired.

<TableBuilder tag={require('@site/docs/components/_dwc_control_map.json').AppNavItem} table='parts' exclusions=''/>
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 [vale] reported by reviewdog 🐶
[Google.Parens] Use parentheses judiciously.

docs/components/appnav.md Outdated Show resolved Hide resolved
docs/components/appnav.md Show resolved Hide resolved
Copy link
Member

@MatthewHawkins MatthewHawkins left a comment

Choose a reason for hiding this comment

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

Just a couple small changes, otherwise this looks good.

@hyyan This is ready for your review when you have some time

docs/components/appnav.md Outdated Show resolved Hide resolved
Copy link
Member

@MatthewHawkins MatthewHawkins left a comment

Choose a reason for hiding this comment

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

Looking better - please review and fix as many of the Vale comments as possible, and then re-request my review. Thanks!


The `AppNavItem` component provides the `setTarget()` method, which allows you to control the behavior of each navigation item when it's clicked. By default, items open in the current browsing context, but you can customize this with the `NavigationTarget` options:

- **SELF**: This is the default option and opens in the current browsing context.
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 [vale] reported by reviewdog 🐶
[Google.Acronyms] Spell out 'SELF', if it's unfamiliar to the audience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not an acronym leaving as is

The `AppNavItem` component provides the `setTarget()` method, which allows you to control the behavior of each navigation item when it's clicked. By default, items open in the current browsing context, but you can customize this with the `NavigationTarget` options:

- **SELF**: This is the default option and opens in the current browsing context.
- **BLANK**: Opens the item in a new tab or window based on browser settings.
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 [vale] reported by reviewdog 🐶
[Google.Acronyms] Spell out 'BLANK', if it's unfamiliar to the audience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not an acronym leaving as is


- **SELF**: This is the default option and opens in the current browsing context.
- **BLANK**: Opens the item in a new tab or window based on browser settings.
- **PARENT**: Opens in the parent browsing context; if there’s no parent, it behaves like SELF.
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 [vale] reported by reviewdog 🐶
[Google.Acronyms] Spell out 'SELF', if it's unfamiliar to the audience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not an acronym leaving as is

Copy link
Member

@MatthewHawkins MatthewHawkins left a comment

Choose a reason for hiding this comment

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

Looks good Lauren - @hyyan this is ready for your review now :)

Copy link
Member

@hyyan hyyan left a comment

Choose a reason for hiding this comment

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

@laurenic0l I've updated the requirements for #261. Please align the demos and the content with the updated requirements.

@hyyan hyyan self-requested a review December 10, 2024 11:53
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.

Document AppNav and AppNavItem components
3 participants