feat(components): add User component#252
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a new LumexUser component with slots and styles, integrates it into docs (usage and custom description examples, preview code wrappers, and a full docs page), updates docs navigation to include LumexUser, and introduces component tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as App/Docs Page
participant LU as LumexUser
participant ST as Styles.User
participant TM as TwMerge/TwVariants
Dev->>LU: Render LumexUser(Name, Description, AvatarContent, DescriptionContent, IsFocusable, Classes)
LU->>ST: Style(TwMerge)
ST->>TM: Build ComponentVariant (slots, variants)
TM-->>ST: ComponentVariant
ST-->>LU: Variant (slot builders)
LU->>LU: Resolve classes per slot (Base, Wrapper, Name, Description)
alt DescriptionContent provided
LU->>Dev: Render DescriptionContent
else
LU->>Dev: Render Description text
end
LU-->>Dev: Markup with data-slot attributes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| Style="@RootStyle" | ||
| data-slot="base" | ||
| data-focus="" | ||
| data-focus-visible="" |
There was a problem hiding this comment.
I see on the HeroUI docs that this two attributes are set, I don't know if they can be useful to this library
There was a problem hiding this comment.
They are setting interaction states programmatically. We don't do this.
These should be covered by Styles.Utils.FocusVisible in the styles. Search for the FocusVisible usages.
There was a problem hiding this comment.
Perfect, I was already using that, I was only wandering if this was the right path. So the IsFocusable property, can be used to toggle this behavior or should I remove it directly?
There was a problem hiding this comment.
Oh, I haven't noticed this prop before. According to HeroUI isFocusable means: "Whether the user is focusable. This is useful when using Dropdown or similar components."
I think it's better to keep it to toggle this behaviour.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #252 +/- ##
==========================================
- Coverage 96.95% 93.28% -3.67%
==========================================
Files 70 151 +81
Lines 1542 2635 +1093
Branches 150 393 +243
==========================================
+ Hits 1495 2458 +963
- Misses 28 92 +64
- Partials 19 85 +66 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// <summary> | ||
| /// Gets or sets the content to render when using a custom description element. | ||
| /// </summary> | ||
| [Parameter] public RenderFragment? DescriptionContent { get; set; } |
There was a problem hiding this comment.
We can merge Description and DescriptionContent all into a render fragment? Or it's better to leave the separated, giving the developer the possibility to choose wheter pass a text or a component?
There was a problem hiding this comment.
Good question. I tend to separate these for the sake of flexibility, yes. This is something we might want to merge in the future though. For now, we should stay consistent with other components that have a similar API.
| } | ||
| </span> | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
I saw you have done this for the LumexAvatar. I've overcomplicated the code or should I leave it as it is?
There was a problem hiding this comment.
I usually extract code into a separate render method when the logic is complex (like the fallback in the avatar) or when the rendering logic involves loops.
So, here is no need for this. See alert component for the reference. Note that RenderFragment goes first in the condition.
| /// <summary> | ||
| /// Gets or sets the CSS class names for the user slots. | ||
| /// </summary> | ||
| [Parameter] public UserSlots? Classes { get; set; } |
There was a problem hiding this comment.
Here is a missing property for the 'Avatar Props', but I'm unsure how to handle the class. Should I use a dedicated class? Or there are already some examples in the codebase?
There was a problem hiding this comment.
I need to think about that...
There was a problem hiding this comment.
This is actually sad :D I see a couple of options:
- Create the
AvatarPropsclass and pass it as parameter. Flaw: Avatar props duplicates + need to always keep them synced; - Create the
AvatarPropsparameter of typeDictionary<string, object>and pass it via attribute splatting (@attributes=...) toLumexAvatar. Flaw: we lose strict typing and intelliSense; - Create the
AvatarContentparameter of typeRenderFragmentto allow consumers define avatars themselves.
I think that the last option is the best here.
I have plans to move to the following structure in the future:
<Alert>
<Alert.Icon />
<Alert.Content>
<Alert.Title>Success</Alert.Title>
<Alert.Description>Your changes have been saved.</Alert.Description>
</Alert.Content>
<Alert.Close />
</Alert>
, where each slot is a separate component that can be styled and arranged independently.
This is something shadcn has been doing for a while alredy, and HeroUI v3 moved to the same concept. With this approach we will gain maximum flexibility as well.
There was a problem hiding this comment.
Actually, I just realized that LumexChip already does that (AvatarContent approach) :D Can't remember own library solutions haha
There was a problem hiding this comment.
But don't wrap AvatarContent with the condition as I did -- it does not make sense.
There was a problem hiding this comment.
I have plans to move to the following structure in the future, where each slot is a separate component that can be styled and arranged independently.
Oh fantastic! I love this approach 🚀
Ok, so for now I add this 'AvatarContent' as RenderFragment
There was a problem hiding this comment.
Is it cool for you, if after this component is reviewed I make a branch to try and convert all the components with old styling method to the new? Just to have the project prepared for the architectural shift. I'd like to help, but I don't want to be too overwhelming 😁
There was a problem hiding this comment.
Is it cool for you, if after this component is reviewed I make a branch to try and convert all the components with old styling method to the new? Just to have the project prepared for the architectural shift. I'd like to help, but I don't want to be too overwhelming 😁
Yes, I’m totally fine with it! I really appreciate your input. I will do my best to make the process as smooth as possible for you.
I have to let you know that the utility for the new styling method is far from ideal, so we’ll most likely need to update it a bit.
| /// <summary> | ||
| /// Gets or sets the content to render when using a custom name element. | ||
| /// </summary> | ||
| [Parameter] public RenderFragment? NameContent { get; set; } |
There was a problem hiding this comment.
I think that we can remove this for the time being to follow HeroUI's API.
They define name as just string. Basically, if some prop in HeroUI is string -- we follow it. If some prop is ReactNode or string | ReactNode -- we do both string and RenderFragment separately.
There was a problem hiding this comment.
Good, I didn't saw that it was only string. Probably I've confused it with the description. I'll fix it right away
| /// <inheritdoc/> | ||
| protected override void OnParametersSet() | ||
| { | ||
| if( ( Description is not null ) && ( DescriptionContent is not null ) ) |
There was a problem hiding this comment.
You can remove this check. I recently realized that it's not good to throw in such cases because passing both won't break anything. In the future, this should be refactored everywhere.
We just need to notify consumers that *Content takes precedence. See https://lumexui.org/docs/components/alert#usage
|
I'm not sure about the last commit, but it's the first thing that has come to my mind about the focus. If it's wrong I'll revert the changes |
Good job -- I would do the same! |
|
For this last commit, I've found out that I was missing the link to the component API even in the 'LumexCode' and 'LumexKbd' components. I've updated the respective branches. Should I open another PR or it's possible for you to implement/retrieve these changes directly? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
src/LumexUI/Components/User/UserSlots.cs (1)
15-31: Consider sealing the slots class.Slots containers are not intended for inheritance; sealing avoids accidental subclassing and enables minor JIT optimizations.
Apply:
-[ExcludeFromCodeCoverage] -public class UserSlots : SlotBase +[ExcludeFromCodeCoverage] +public sealed class UserSlots : SlotBasedocs/LumexUI.Docs.Client/Pages/Components/User/Examples/Usage.razor (1)
1-5: Add alt text for the avatar (a11y) and consider a local image to avoid flaky docs.Alt improves screen‑reader UX; local assets reduce external HTTP reliance.
- <LumexAvatar Src="https://i.pravatar.cc/300" /> + <LumexAvatar Src="https://i.pravatar.cc/300" Alt="Jane Doe avatar" />docs/LumexUI.Docs.Client/Pages/Components/User/Examples/LinkDescription.razor (1)
1-10: Add alt text for the avatar (a11y).Minor, but helpful for accessibility.
- <LumexAvatar Src="https://avatars.githubusercontent.com/u/68395709?v=4" /> + <LumexAvatar Src="https://avatars.githubusercontent.com/u/68395709?v=4" Alt="Dan avatar" />tests/LumexUI.Tests/Components/User/UserTests.razor (2)
23-27: Fix typo in sample description.Spelling: “Engineer”.
- @<LumexUser Name="John Doe" Description="Software Engeneer" /> + @<LumexUser Name="John Doe" Description="Software Engineer" />
30-46: Optional: add a focused-style test to cover the IsFocusable variant.Covers the variant wiring and style application.
[Fact] public void ShouldRenderDescriptionContent() { @@ cut.FindByTestId("user-link").Should().NotBeNull(); } + + [Fact] + public void ShouldApplyFocusableStylesWhenEnabled() + { + var cut = Render( + @<LumexUser Name="John Doe" IsFocusable="true" data-testid="user-root" /> + ); + + var root = cut.FindByTestId("user-root"); + root.Should().NotBeNull(); + root.GetAttribute("class").Should().Contain("focus-visible:outline-2"); + }If Base is not the root node, adjust the testid to target the Base element.
docs/LumexUI.Docs.Client/Pages/Components/User/User.razor (2)
25-27: Fix “Class/Classes” description to match implementationRoot Class is merged into the Base slot, while per-slot overrides live in Classes.*. Update the copy to avoid confusion.
Apply this diff:
- <li><code>Class</code>: The basic CSS class name styles the wrapper of the user contents.</li> - <li><code>Classes</code>: The CSS class names for the user slots style the entire user at once.</li> + <li><code>Class</code>: CSS classes merged into the root element (base slot).</li> + <li><code>Classes</code>: Per‑slot CSS class overrides (<code>Base</code>, <code>Wrapper</code>, <code>Name</code>, <code>Description</code>).</li>
13-19: Add an accessibility note about IsFocusableIsFocusable only toggles focus-visible styles. Consumers still need a focusable root (e.g., As="button") or tabindex="0" on a non-interactive element.
Apply this diff:
</LumexAlert> + <LumexAlert Radius="@Radius.Large" Class="my-4"> + <DescriptionContent> + Accessibility: <code>IsFocusable</code> only affects styles. For keyboard focus when using a non‑interactive root (e.g., <code>div</code>), set <code>As="button"</code> or provide <code>tabindex="0"</code> via <code>AdditionalAttributes</code>. + </DescriptionContent> + </LumexAlert>src/LumexUI/Components/User/LumexUser.razor.cs (5)
15-16: Tighten summary wordingThe component exposes Name/Description; “email” isn’t part of the API here.
-/// A component that represents user information, such as an avatar, name, and email. +/// A component that represents user information, such as an avatar, name, and description.
39-42: Fix doc comment grammarMinor typo.
-/// Gets or sets the whether the user is focusable. +/// Gets or sets whether the user is focusable.
64-67: Use a more appropriate exception with contextNotImplementedException is misleading here. Prefer InvalidOperationException (or KeyNotFoundException) with a clear message.
- if( !_slots.TryGetValue( slot, out var styles ) ) - { - throw new NotImplementedException(); - } + if( !_slots.TryGetValue( slot, out var styles ) ) + { + throw new InvalidOperationException($"Unknown slot '{slot}' for {nameof(LumexUser)}."); + } @@ - _ => throw new NotImplementedException() + _ => throw new InvalidOperationException($"Unknown slot '{slot}' for {nameof(LumexUser)}.")Also applies to: 75-76
49-59: Minor: cache Style() result locally/staticStyle() already caches internally, but you can keep a private field to avoid re-fetching on every parameters set (micro). Optional.
- protected override void OnParametersSet() - { - var user = Styles.User.Style( TwMerge ); - _slots = user( new() - { - [nameof( IsFocusable )] = IsFocusable.ToString(), - } ); - } + private ComponentVariant? _userVariant; + + protected override void OnParametersSet() + { + _userVariant ??= Styles.User.Style( TwMerge ); + _slots = _userVariant( new() + { + [nameof( IsFocusable )] = IsFocusable.ToString(), + } ); + }
22-33: Optional: consider an Avatar slot in the futureIf consumers need to style/target the avatar container via the slot system, introducing an Avatar slot (and Classes.Avatar) would align with the other slots. Non-blocking and can wait for the multi-component slot refactor you plan.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/LumexUI.Docs.Client/Common/Navigation/NavigationStore.cs(2 hunks)docs/LumexUI.Docs.Client/Pages/Components/User/Examples/LinkDescription.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/User/Examples/Usage.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/User/PreviewCodes/LinkDescription.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/User/PreviewCodes/Usage.razor(1 hunks)docs/LumexUI.Docs.Client/Pages/Components/User/User.razor(1 hunks)src/LumexUI/Components/User/LumexUser.razor(1 hunks)src/LumexUI/Components/User/LumexUser.razor.cs(1 hunks)src/LumexUI/Components/User/UserSlots.cs(1 hunks)src/LumexUI/Styles/User.cs(1 hunks)tests/LumexUI.Tests/Components/User/UserTests.razor(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/LumexUI/Components/User/UserSlots.cs (2)
src/LumexUI/Styles/User.cs (1)
ExcludeFromCodeCoverage(13-54)src/LumexUI/Components/User/LumexUser.razor.cs (1)
ExcludeFromCodeCoverage(61-77)
docs/LumexUI.Docs.Client/Common/Navigation/NavigationStore.cs (1)
src/LumexUI/Components/User/LumexUser.razor.cs (1)
LumexUser(17-78)
src/LumexUI/Styles/User.cs (6)
src/LumexUI/Components/User/LumexUser.razor.cs (2)
ExcludeFromCodeCoverage(61-77)LumexUser(17-78)src/LumexUI/Components/User/UserSlots.cs (1)
ExcludeFromCodeCoverage(14-31)src/LumexUI/Utilities/Variants/SlotCollection.cs (1)
SlotCollection(9-34)src/LumexUI/Utilities/Variants/VariantCollection.cs (1)
VariantCollection(9-34)src/LumexUI/Utilities/Variants/VariantValueCollection.cs (1)
VariantValueCollection(9-34)src/LumexUI/Styles/Utils.cs (1)
Utils(5-28)
src/LumexUI/Components/User/LumexUser.razor.cs (4)
src/LumexUI/Components/Bases/LumexComponentBase.cs (1)
LumexComponentBase(18-58)src/LumexUI/Utilities/Variants/TwVariants.cs (1)
ComponentSlot(112-115)src/LumexUI/Styles/User.cs (1)
ExcludeFromCodeCoverage(13-54)src/LumexUI/Components/User/UserSlots.cs (1)
ExcludeFromCodeCoverage(14-31)
🔇 Additional comments (11)
src/LumexUI/Components/User/UserSlots.cs (1)
11-31: LGTM — slots DTO looks consistent with the slot infra.Docs are clear, nullability is correct, and it aligns with GetStyles usage across slots.
Please confirm SlotBase exposes Base so Classes?.Base works end-to-end.
docs/LumexUI.Docs.Client/Pages/Components/User/PreviewCodes/Usage.razor (1)
1-5: LGTM — matches the existing PreviewCode pattern.Confirm @rendermode is consistent with other User previews for a uniform DX.
docs/LumexUI.Docs.Client/Pages/Components/User/PreviewCodes/LinkDescription.razor (1)
1-5: LGTM — consistent with the Usage preview.src/LumexUI/Styles/User.cs (3)
7-11: Resolve potential missing usings for type references.If there isn’t a global using for UserSlots/LumexUser, this will not compile.
using LumexUI.Utilities; +using LumexUI; // for UserSlots +using LumexUI.Components.User; // for LumexUser using TailwindMerge;
16-23: Cache ties the first TwMerge instance to the variant.If TwMerge isn’t a singleton, later calls won’t pick up a different instance/config.
Consider either (a) guaranteeing TwMerge is singleton in DI, or (b) keying the cache by the TwMerge instance and recreating when it changes.
44-51: Focus styles require a focusable element.Ensure the Base slot renders on a focusable element or sets tabindex when IsFocusable=true; otherwise the variant has no effect.
docs/LumexUI.Docs.Client/Common/Navigation/NavigationStore.cs (1)
49-51: New nav entries: verify target pages/routes exist (LumexUser verified; LumexTooltip outstanding)
- LumexUser — verified: docs/LumexUI.Docs.Client/Pages/Components/User/User.razor contains @page "/docs/components/user".
- LumexTooltip — not verified here; ensure a corresponding page/route and correct filename/casing exist to avoid broken links.
docs/LumexUI.Docs.Client/Pages/Components/User/User.razor (2)
6-12: Solid docs section scaffoldingUsage + example previews read well and match the component surface.
57-66: Use full component name for ComponentLinksProps id — replace "Usr" with "User" or confirm shorthand is intentional.
Location: docs/LumexUI.Docs.Client/Pages/Components/User/User.razor (linksProps: new ComponentLinksProps("Usr", isServer: true), line 64).src/LumexUI/Components/User/LumexUser.razor (2)
6-11: Root wrapper looks goodProps/attrs are correctly forwarded to LumexComponent and the base slot class is applied.
14-27: Avoid rendering empty spans when no contentSkip Name/Description wrappers when the corresponding content is empty to reduce DOM noise.
File: src/LumexUI/Components/User/LumexUser.razor (lines 14-27)
- <span class="@GetStyles( nameof( S.Name ) )" data-slot="name"> - @Name - </span> + @if (!string.IsNullOrWhiteSpace(Name)) + { + <span class="@GetStyles( nameof( S.Name ) )" data-slot="name"> + @Name + </span> + } - <span class="@GetStyles( nameof( S.Description ) )" data-slot="description"> - @if ( DescriptionContent is not null ) - { - @DescriptionContent - } - else - { - @Description - } - </span> + @if (DescriptionContent is not null || !string.IsNullOrWhiteSpace(Description)) + { + <span class="@GetStyles( nameof( S.Description ) )" data-slot="description"> + @if ( DescriptionContent is not null ) + { + @DescriptionContent + } + else + { + @Description + } + </span> + }Run locally to confirm there are no tests depending on these wrappers:
rg -n -C2 'data-slot="(description|name)"' tests || git grep -n 'data-slot="(description|name)"'
I also constantly forget to add the API links 😄 I think there might be more missing, so it’s better to create a single PR to add them all. You can do this if you want. I'd immediately approve and merge. |
|
Thank you very much for your contribution! <3 P.S. Sorry, I just realized I forgot to thank you.. |
2 participants
Description
This PR introduces a new LumexUser component, including its initial implementation, styling, usage properties, and accompanying documentation.
What's been done?
Checklist
Additional Notes
I'd like to take this PR more as a draft, to ask you questions about possible implementations of properties and refinements.
Summary by CodeRabbit
New Features
Documentation
Tests