Skip to content

Conversation

@ditadi
Copy link
Contributor

@ditadi ditadi commented Mar 24, 2024

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests
  • Other

Why is it needed?

This PR adds a styled breadcrumb component. This way we can improve the navigation experience on applications using qwik-ui and increase the variety of components in the library.

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have ran pnpm change and documented my changes
  • I have add necessary docs (if needed)
  • Added new tests to cover the fix / functionality

@changeset-bot
Copy link

changeset-bot bot commented Mar 24, 2024

🦋 Changeset detected

Latest commit: 887689e

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

This PR includes changesets to release 1 package
Name Type
@qwik-ui/styled Patch

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
Copy link
Contributor

github-actions bot commented Mar 24, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@ditadi
Copy link
Contributor Author

ditadi commented Mar 24, 2024

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Mar 24, 2024
@thejackshelton
Copy link
Collaborator

thejackshelton commented Mar 26, 2024

Looks good! @maiieul if you could double check since you work more on the styled kit, otherwise happy to merge.

@maiieul
Copy link
Contributor

maiieul commented Mar 26, 2024

First off, thanks a ton for this great PR @ditadi! I wouldn't have done such a great job with the Comp trick 👌

My first impression is that the breadcrumb doesn't feel brutal enough in brutalist style. So I added a small commit to try and fix that. Would be awesome if you could review my changes @ditadi 🫶. It's the first time I commit on top of someone else's PR. We can always revert it if you (or anyone else) is against the changes.

Also small nit pick: do you guys see any advantages in doing

type BreadcrumbSeparatorProps = PropsOf<'li'>;
const BreadcrumbSeparator = component$<BreadcrumbSeparatorProps>((props) => {
  return (
    <li
      role="presentation"
      aria-hidden="true"
      class={cn(
        '[&>svg]:stroke-muted-foreground [&>svg]:size-3.5 [&>svg]:stroke-2',
        props.class,
      )}
      {...props}
    >
      <LuChevronRight />
    </li>
  );
});

Over

type BreadcrumbSeparatorProps = PropsOf<'li'>;
const BreadcrumbSeparator = component$<BreadcrumbSeparatorProps>((props) => {
  return (
    <li role="presentation" aria-hidden="true" {...props}>
      <LuChevronRight class="stroke-muted-foreground size-3.5 stroke-2" />
    </li>
  );
});

@ditadi
Copy link
Contributor Author

ditadi commented Mar 26, 2024

First off, thanks a ton for this great PR @ditadi! I wouldn't have done such a great job with the Comp trick 👌

My first impression is that the breadcrumb doesn't feel brutal enough in brutalist style. So I added a small commit to try and fix that. Would be awesome if you could review my changes @ditadi 🫶. It's the first time I commit on top of someone else's PR. We can always revert it if you (or anyone else) is against the changes.

Also small nit pick: do you guys see any advantages in doing

type BreadcrumbSeparatorProps = PropsOf<'li'>;
const BreadcrumbSeparator = component$<BreadcrumbSeparatorProps>((props) => {
  return (
    <li
      role="presentation"
      aria-hidden="true"
      class={cn(
        '[&>svg]:stroke-muted-foreground [&>svg]:size-3.5 [&>svg]:stroke-2',
        props.class,
      )}
      {...props}
    >
      <LuChevronRight />
    </li>
  );
});

Over

type BreadcrumbSeparatorProps = PropsOf<'li'>;
const BreadcrumbSeparator = component$<BreadcrumbSeparatorProps>((props) => {
  return (
    <li role="presentation" aria-hidden="true" {...props}>
      <LuChevronRight class="stroke-muted-foreground size-3.5 stroke-2" />
    </li>
  );
});

@maiieul
About your PR:
Not a problem at all, feel free to do it on another opportunity.

About your Change:
Much better, I will talk more with you about this brutalist style next time.

About the Style over <li> or SVG:
I think both option has advantages. Passing the style to the SVG makes sense because is the component that is rendered, so is more cohesive. But, adding the styles on the <li> is also interesting to ensure we will have the same style, it doesn't matter the icon. Is a little bit about preferences, sending the style to the SVG made the component cleaner, so I'm ok with that.

@maiieul
Copy link
Contributor

maiieul commented Mar 26, 2024

Thanks for your feedback!

Yeah I think the styles on the Icon's class are better in this case. Having them on the li would be useful if it was a Slot imo.

@maiieul maiieul merged commit 2afd090 into qwikifiers:main Mar 27, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants