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

Added withOnSelectedHandlers #81

Merged
merged 4 commits into from
Feb 13, 2025
Merged

Added withOnSelectedHandlers #81

merged 4 commits into from
Feb 13, 2025

Conversation

vlipovetskii
Copy link
Contributor

withOnSelectedHandlers is a utility that facilitates event handling when a tab is selected.

[withOnSelectedHandlers] is a utility that facilitates event handling when a tab is selected.
@mvysny
Copy link
Owner

mvysny commented Feb 4, 2025

I'm kinda hesitant to pull this PR, for multiple reasons:

  1. If the purpose is only to show certain content when a tab is selected, TabSheet is a perfect fit for that
  2. It adds new API (Tab.onSelected{}) in a bit of a complex way. A cleaner approach would be to open a feature request at Vaadin flow-components github repo to add the necessary function right into Tab itself.

@mvysny mvysny self-assigned this Feb 4, 2025
@mvysny mvysny added the more info needed the reporter needs to provide more information in order to fix the bug label Feb 4, 2025
@vlipovetskii
Copy link
Contributor Author

vlipovetskii commented Feb 4, 2025

  1. TabSheet is not acceptable. The Tabs are located in Drawer and NavigationBar and manage AppLayout content.
  2. Feature request to Vaadin repo is a very long story. So, could we consider the following options?
    2.1. To leverage this function just inside my applications without sharing it with other developers, which use karibu-dsl.
    2.2. To share this function with other developers, using karibu-dsl, and in the future replace with onSelected if and when Vaadin adds it to Tab component.

p.s. To be honest )), I like FP and it was a pleasure for me to build the implementation without introducing new classes, just with help of high-order functions.

@mvysny
Copy link
Owner

mvysny commented Feb 6, 2025

Got it. I'm a fan of FP too; for example I definitely prefer the simplicity of db{} function over having Spring and all of its DI and AOP madness. I quite like using the simplest bits of OOP and FP in a way that's simple and easily understandable by others.

Your withOnSelectedHandlers{} function is a bit of a novelty for me; let me grab some time and think about it. The idea is really intriguing, since you're adding both a state and a scoped API to a component, but not directly, but via a local variable captured by a closure. Let me digest it a bit :-D Let's discuss this novel approach a bit here.

One of the problems I see is as follows: say you'll have another function withSomeOtherKindOfHandlers{} doing similar job; to use both APIs you'll need to structure your code in this kind of a way:

tabs {
     withOnSelectedHandlers { onSelected ->
        withSomeOtherKindOfHandlers { handler ->
          withYetSomethingElse { handler2 ->
             tab("Users") {
                onSelected {
                    displayUserList()
               }
            }
         tab("Admins") {
             onSelected {
                 displayAdminList()
             }
         }

Maybe I'm just going ahead too much of myself, but this can happen if this approach is applied to other components as well; and this starts to resemble the callback pyramid of doom too painfully.

Also, the state (the onSelectedHandlers HashMap) is really well hidden within the closure and can not be accessed outside, via a getter or such. Not saying there's a need for that now; just freely thinking about this kind of approach and maybe using it in the future too.

The withOnSelectedHandlers{} function is subjectively a bit complex - for someone who reads the code, it's not easy to see what's going on within the function.

Maybe I would do this a bit differently:

  1. Store the state into Tabs via ComponentUtil.setData(). That way, we can get rid of withOnSelectedHandlers{} and the potential pyramid of doom when adding layers of state.
  2. I think Karibu-DSL offers a Tabs.tab extension property which retrieves Tabs from Tab, so that should be helpful with locating the HashMap state variable.
  3. The Tab. onSelected() can then be refactored as a simple extension function on Tab.

I think this solution is a bit simpler and easier to understand by our fellow programmers. What do you think?

@vlipovetskii
Copy link
Contributor Author

vlipovetskii commented Feb 6, 2025

I quite like using the simplest bits of OOP and FP in a way that's simple and easily understandable by others.

So do I! I would add that following KISS (e.g. combining the simplest bits OOP and FP) aid even to ourselves to understand what we did when the time has passed.

  1. Store the state into Tabs via ComponentUtil.setData(). That way, we can get rid of withOnSelectedHandlers{} and the potential pyramid of doom when adding layers of state.
  2. I think Karibu-DSL offers a Tabs.tab extension property which retrieves Tabs from Tab, so that should be helpful with locating the HashMap state variable.
  3. The Tab. onSelected() can then be refactored as a simple extension function on Tab.

I think this solution is a bit simpler and easier to understand by our fellow programmers. What do you think?

Wow, that's the great approach! I have not known about ComponentUtil.setData(). I will think how to re-design, following your advices.

@mvysny mvysny merged commit fe2b4b1 into mvysny:master Feb 13, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more info needed the reporter needs to provide more information in order to fix the bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants