Skip to content

Conversation

susanu
Copy link
Contributor

@susanu susanu commented Jul 19, 2024

WHY

If you have the nest action buttons within a dropdown enabled globally, you probably have some cruds with just 1 action button and you don't want a dropdown with just a button.

BEFORE - What was wrong? What was happening before this PR?

No matter the number of action buttons, the links were nested within a dropdown.

AFTER - What is happening after this PR?

Give the ability to disable the dropdown by setting the minimum threshold for buttons.

HOW

How did you achieve that, in technical terms?

Added a minimum threshold and check the number of buttons before nesting.

Is it a breaking change?

No, because the threshold is set to 1 which preserves the current behavior

How can we test the before & after?

Create a CRUD with 2 operations, list and create/update/delete or show

@jcastroa87
Copy link
Member

Hello @susanu

Thanks a lot for this, i will ask @pxpm to check and merge if everything is ok.

Thanks again.

Cheers.

@jcastroa87 jcastroa87 requested a review from pxpm July 19, 2024 14:15
@pxpm
Copy link
Contributor

pxpm commented Jul 19, 2024

Hey @susanu thanks a lot for the PR. This is mostly what I had in mind, good job 🙏

I think in terms of implementation there is not much to change here, but like I said I want to keep the possibility of extending this functionality open in the future.
One of the things I would like to consider in a future implementation is something in the likes of: lineButtonsAsDropdown**Minimum**Threshold, for situations when you want to force say 2 links to appear on the page, and the rest as a dropdown. Does not matter if entry has 3, 4 ,5 actions, first two appear on page, the rest on the dropdown.

I am thinking in terms of naming mostly, and how we could potentially frame it in a future improvement here to consider both scenarios I've named.

I will discuss this with the rest of the team and get back to you.

Once again, thanks, I think this is a very clean and easy to maintain PR. 🙏

Cheers

@pxpm
Copy link
Contributor

pxpm commented Sep 18, 2024

Hey @susanu I've created #5667 by pulling this PR and adding the changes I proposed previously.

I think it now is flexible enough for multiple use cases. I've renamed the things a bit here and there.
Mind giving your opinion on the PR ? Does it look good to you ?

Thank you very much for pushing this foward 🙏

Cheers

@pxpm pxpm closed this Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants