Skip to content

feat: sd navigation item#377

Merged
yoezlem merged 32 commits intomainfrom
feat/sd-navigation-item
Sep 29, 2023
Merged

feat: sd navigation item#377
yoezlem merged 32 commits intomainfrom
feat/sd-navigation-item

Conversation

@abudd1094
Copy link
Copy Markdown
Contributor

@abudd1094 abudd1094 commented Sep 12, 2023

My first attempt at a Solid PR for ticket:
closes #346

Please help me to adhere to the contribution guidelines and ensure quality stories / testing / organization.

@mariohamann
Copy link
Copy Markdown
Contributor

CleanShot 2023-09-13 at 09 31 38@2x

On Safari the details' arrow is showing up.

@mariohamann
Copy link
Copy Markdown
Contributor

Question for @solid-design-system/design

CleanShot 2023-09-13 at 09 30 18@2x

Should it possible that smaller or bigger nav-items have a chevron? Should we change the size of the chevron then?

@mariohamann
Copy link
Copy Markdown
Contributor

Please remove this kind of stuff from the stories. This is usually only needed when we have multiple kinds of stories in one file, but not for documentation purpose. For documentation you can use the comments above the story.

CleanShot 2023-09-13 at 09 39 59@2x

@mariohamann
Copy link
Copy Markdown
Contributor

We need stories for relaxed and indented. Please set the element to fixed width then.

@mariohamann
Copy link
Copy Markdown
Contributor

CleanShot 2023-09-13 at 09 46 23@2x

We only provide default, description and children slot. Please remove the others. (You can create every example by just filling the default slot.)

@van-nguyen-ht
Copy link
Copy Markdown
Contributor

van-nguyen-ht commented Sep 13, 2023

Hi Mario,
small slot/default - as it's used for level 3 content - there's no nested content for that so I don't think a smaller chevron is needed there
Here's what Ella @EllaELSA commented "Level 3 is smaller text, regular, bold in selected, no description and no icon, no arrow, no badge"

@mariohamann
Copy link
Copy Markdown
Contributor

CleanShot 2023-09-13 at 09 49 23@2x

mouseenter, mouseleave, click etc. can easily be done on the whole element from users of the element, we don't have to emit that ourself.

Instead I would send events sd-show and sd-hide for the accordion behaviour, as we do for sd-accordion.

@mariohamann
Copy link
Copy Markdown
Contributor

mariohamann commented Sep 13, 2023

Please add at least the following props to make it usable as a link:

target, download

See sd-button for imlementation details.

@mariohamann
Copy link
Copy Markdown
Contributor

mariohamann commented Sep 15, 2023

Nice! That looks pretty fantastic.

Some small stuff:

CleanShot 2023-09-15 at 16 46 52@2x
  • Please use our default focus style for this element. I think it's something like focus-visible:focus-outline or similar.
CleanShot 2023-09-15 at 16 47 50@2x
  • Please remote the size part in the variant x size story from the accordion, as this shouldn't be used from a UX perspective (even if it's technically possible).

CleanShot 2023-09-15 at 16 48 41

  • Please make a fixed/bigger width for nav items, so it doesn't jump when collapsing.
CleanShot 2023-09-15 at 16 50 12@2x
  • We use semantic versioning, so we're just doing a minor bump. Just wait until you finally merge and make a single commit beofre that, which uses e. g. 1.14 or whatever is the next version than.

  • @coraliefeil Currently the icon-only option is missing – I would like to make this as an improvement within a next ticket, so Alec can can close this one here sooner.

  • @van-nguyen-ht If you have updated on the "current" line on the left, we would need that now. :)

@mariohamann
Copy link
Copy Markdown
Contributor

mariohamann commented Sep 18, 2023

@abudd1094

Sizes should be sm base lg instead of smaller and larger. (Design changed the wording so we can use the same.)

* @summary Flexible button / link component that can be used to quickly build navigations. Takes one of 3 forms: link (overrides all other if 'href' is provided), button (default), or accordion (if 'children' slot defined).
* @status experimental
* @since 1.0
* @since 2.11.0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

pls only bump the minor version.

@karlbaumhauer karlbaumhauer removed their assignment Sep 20, 2023
@coraliefeil
Copy link
Copy Markdown
Contributor

I’m a bit confused now what to do after I reviewed sth and denied stories ... approve PR or not?

@karlbaumhauer
Copy link
Copy Markdown
Contributor

I’m a bit confused now what to do after I reviewed sth and denied stories ... approve PR or not?

Bildschirmfoto 2023-09-20 um 15 37 48

As you denied 2 stories, the check failed and the PR cannot be merged. As soon as changes have been pushed, you can re-review it and as soon as you approved it, the check will pass and the PR can be merged.

@yoezlem
Copy link
Copy Markdown
Contributor

yoezlem commented Sep 26, 2023

@abudd1094 whats the status of this Ticket?

@abudd1094
Copy link
Copy Markdown
Contributor Author

@abudd1094 whats the status of this Ticket?

All feedback has been resolved. Ticket is updated and ready for re-review.

@mariohamann
Copy link
Copy Markdown
Contributor

@solid-design-system/design Could you have a look again? :)

@yoezlem yoezlem merged commit 25af69b into main Sep 29, 2023
@yoezlem yoezlem deleted the feat/sd-navigation-item branch September 29, 2023 10:48
karlbaumhauer pushed a commit that referenced this pull request Sep 29, 2023
# [@solid-design-system/components-v1.15.0](components/1.14.4...components/1.15.0) (2023-09-29)

### Bug Fixes

* 🐛 provide docs how to use registerIconLibrary with UMD bundle ([#433](#433)) ([0b95a21](0b95a21))

### Features

* sd navigation item ([#377](#377)) ([25af69b](25af69b))
yoezlem pushed a commit that referenced this pull request Oct 9, 2023
Co-authored-by: Alec Budd <alec.budd@prodyna.com>
yoezlem pushed a commit that referenced this pull request Oct 9, 2023
# [@solid-design-system/components-v1.15.0](components/1.14.4...components/1.15.0) (2023-09-29)

### Bug Fixes

* 🐛 provide docs how to use registerIconLibrary with UMD bundle ([#433](#433)) ([0b95a21](0b95a21))

### Features

* sd navigation item ([#377](#377)) ([25af69b](25af69b))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

feat: ✨ add sd-navigation-item

9 participants