Skip to content

Conversation

MarvinKlein1508
Copy link
Collaborator

Pull Request

📖 Description

This PR adds the FluentDragContainer and FluentDropZone components from v4 branch to v5. They are basically a copy & paste with just small changes to get them working in v5.

I also added the demo section and adjusted both test cases.

🎫 Issues

Implements #4208

👩‍💻 Reviewer Notes

@vnbaaij & @dvoituron not sure if you wanted to change something fundamental here in v5.

📑 Test Plan

✅ Checklist

General

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • 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
  • I have modified an existing component
  • I have validated the Unit Tests for an existing component

⏭ Next Steps

I think in v5 we want to add test cases for each property right?

Copy link
Collaborator

@vnbaaij vnbaaij left a comment

Choose a reason for hiding this comment

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

The components must have @attributes in their razor files and use the v5 default class and style builders. With the way it is done now, unit tests are failing (because the standard parameters are not being utilized.

The components must be added in the ComponentInitializer dictionary in ComponentBaseTests.cs like this:

{ typeof(FluentDragContainer<>), Loader.MakeGenericType(typeof(string))},
{ typeof(FluentDropZone<>), Loader.MakeGenericType(typeof(string))},

Also, a lot more tests need to be added. We are strict on having 98% line coverage for v5. Current result:

Image

So yes, all properties/parameters must have tests. And also yes, we didn;t have any additional plans for these components for V5.

@MarvinKlein1508
Copy link
Collaborator Author

@vnbaaij I have added your requested changes. Any idea why the ComponentBaseTest is failing?
grafik

@vnbaaij
Copy link
Collaborator

vnbaaij commented Oct 6, 2025

@vnbaaij I have added your requested changes. Any idea why the ComponentBaseTest is failing?

You forgot to use default builders in drag container cs file

@MarvinKlein1508
Copy link
Collaborator Author

MarvinKlein1508 commented Oct 6, 2025

@vnbaaij I have added your requested changes. Any idea why the ComponentBaseTest is failing?

You forgot to use default builders in drag container cs file

oh wow. I forgot to save the file. Should be done now. I will add more UNIT tests later on. Can you tell me how I can see the coverage for the specific component?

Nevermind - the docs got me covered! 🤫

@MarvinKlein1508
Copy link
Collaborator Author

@vnbaaij I need your wisdom once again. This testing stuff is kinda new to me - so please show mercy! 🤣

How would you do it for the Draggable property? I guess I shouldn't declare two methods for true and false, right?

And how can I even check for mouse events? The docs shows how you can do it with OnClick but not with any other.

If you could give me like 1 or 2 examples then I will happily add the rest of the tests.

}

div[dragged-over] {
background-color: lightgray;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to use a FluentUI CSS variable (and not always lightgray) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Which one would you recommend?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dvoituron @vnbaaij

I've set it to

background-color: var(--colorNeutralBackground2);
color: var(--colorNeutralForeground2);

any doubts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't approve the design at this time. But perhaps the colors “--colorNeutralBackground4” and
“--colorNeutralForeground4” could be more visible? To check :-)

@inherits Bunit.TestContext

@code {
public FluentDragTests()
Copy link
Collaborator

Choose a reason for hiding this comment

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

You will probably need to add additional unit tests to achieve at least 98% code coverage (100% if possible).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea I know. Could you help me out by providing a testcase for the Draggable parameter? See: #4211 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have time during these days. But I can check that later.
Perhaps you will find assistance by using GitHub Copilot.

@vnbaaij
Copy link
Collaborator

vnbaaij commented Oct 6, 2025

@vnbaaij I need your wisdom once again. This testing stuff is kinda new to me - so please show mercy! 🤣

How would you do it for the Draggable property? I guess I shouldn't declare two methods for true and false, right?

And how can I even check for mouse events? The docs shows how you can do it with OnClick but not with any other.

If you could give me like 1 or 2 examples then I will happily add the rest of the tests.

I'd need to look into this on how to do it. Maybe you can draw inspiration from FluentMultiSplitterTests?

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