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

[Stack] Added 'Stretch' horizontal alignment option #2800

Merged
merged 8 commits into from
Oct 14, 2024

Conversation

RobMHarris
Copy link
Contributor

Introduced a new horizontal alignment option, Stretch, to the FluentStack component. Updated the HorizontalAlignment enum in HorizontalAlignment.cs to include the Stretch value with a summary comment. Modified the GetHorizontalAlignment method in FluentStack.razor.cs to map HorizontalAlignment.Stretch to the string "stretch".

Pull Request

📖 Description

While using the FluentStack in vertical mode I found that I was unable to set the underlying flexbox "align-items" style to "stretch" as this option was missing from the HorizontalAlignment enum. Therefore I have added "Stretch" to the enum and implemented the option in the GetHorizontalAlignment method of the FluentStack component.
I don't believe this is a breaking change as the default option is still "start".

🎫 Issues

👩‍💻 Reviewer Notes

To view the change in action visit the /Stack page of the demo and in the "Using the FluentStack component" section select the "Stretch" option in the "Horizonal" options.

This is my very first pull request so apologies if I haven't followed any procedures correctly.

📑 Test Plan

There doesn't appear to be any existing test coverage for these areas.

✅ Checklist

General

  • I have added tests for my changes.
  • [x ] I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • [x ] I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

  • I have added a new component
  • I have added Unit Tests for my new component
  • [x ] I have modified an existing component
  • I have validated the Unit Tests for an existing component

⏭ Next Steps

Introduced a new horizontal alignment option, `Stretch`, to the `FluentStack` component. Updated the `HorizontalAlignment` enum in `HorizontalAlignment.cs` to include the `Stretch` value with a summary comment. Modified the `GetHorizontalAlignment` method in `FluentStack.razor.cs` to map `HorizontalAlignment.Stretch`  to the string "stretch".
@vnbaaij
Copy link
Collaborator

vnbaaij commented Oct 12, 2024

Good addition. It would really help us if you could add some better unit test for the Stack (pretty please)..

@RobMHarris
Copy link
Contributor Author

Good addition. It would really help us if you could add some better unit test for the Stack (pretty please)..

No problem, I will have a look at that..

@RobMHarris
Copy link
Contributor Author

@microsoft-github-policy-service agree

@dvoituron
Copy link
Collaborator

I think it's a good idea. Thank you very much.

But I don't know if this will be possible in the current version of Lib, because the enumeration is also used with the Dialog component (which should be updated in v5). So, when adding a new attribute, you need to check what the result will be with this component. And update or document it to explain it.

@vnbaaij
Copy link
Collaborator

vnbaaij commented Oct 12, 2024

Good point Denis. Think we can still do it on this version. We just need to do a check in dialog code to see if this invalid value is not being used. We have that on other components as well (button/anchor from the top of my head)

@dvoituron
Copy link
Collaborator

Yep. But we should add an info message in these component properties to explain that this value is nor applicable.

The Alignment property now includes a custom setter that throws an ArgumentException if set to HorizontalAlignment.Stretch, ensuring this value is not supported.
@RobMHarris
Copy link
Contributor Author

I've added a custom setter for the Aligment property of the DialogParameters class that ensures that a vaue of "stretch" can't be set in this context. Could you maybe suggest a suitable place to add a unit test for this? I can see there's a FluentDialogServiceTests class - maybe here? (Sorry I'm not familiar with BUnit)

@vnbaaij
Copy link
Collaborator

vnbaaij commented Oct 13, 2024

Yes that one or FluentDialogTests

@dvoituron
Copy link
Collaborator

I've added a custom setter for the Aligment property of the DialogParameters class that ensures that a vaue of "stretch" can't be set in this context. Could you maybe suggest a suitable place to add a unit test for this? I can see there's a FluentDialogServiceTests class - maybe here? (Sorry I'm not familiar with BUnit)

Awesome. Can you add this info about "stretch" in the XML documentation (just above public virtual HorizontalAlignment Alignment)?

For the unit test, you could read this page: https://github.com/microsoft/fluentui-blazor/blob/dev/docs/unit-tests.md

@RobMHarris
Copy link
Contributor Author

I've updated the XML documentation and added a new unit test for the invalid DialogParameters.Alignment value.

@vnbaaij vnbaaij enabled auto-merge (squash) October 14, 2024 18:48
@vnbaaij vnbaaij changed the title Added missing 'Stretch' horizontal alignment option to FluentStack [Stack] Added 'Stretch' horizontal alignment option Oct 14, 2024
@vnbaaij vnbaaij merged commit 1a408be into microsoft:dev Oct 14, 2024
4 checks passed
@RobMHarris RobMHarris deleted the HorizontalAlignment-Stretch branch October 14, 2024 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants