-
-
Notifications
You must be signed in to change notification settings - Fork 59
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
Refactor sidebar #415
Refactor sidebar #415
Conversation
@gvreddy04 coul you take a look on this? I really need more levels of navigation in our app :) |
@gvreddy04 here is a render comparison from old code and new code. Notice the slight reduction of rendertime. render_demo.mp4 |
@MarvinKlein1508 Thank you very much for the PR and for your contributions. We should not remove the DataProvider parameter. Instead, we can add support for a Data parameter like Grid. |
@MarvinKlein1508 Enabling more than two levels in the Sidebar will cause UI issues, as demonstrated by the broken feature shown below. There may be other UI scenarios that encounter problems as well. I am considering alternative options for accommodating more than two levels. |
@gvreddy04 oh I haven't though about the toggled menu since I'm not using it. I will take a look on it as well and add more commits to the PR. Can you explain what the purpose of the Delegate is? When ever I need to update the sidebar entries I can just change my collection. |
@gvreddy04 can you maybe add your scenario as demo to the PR? |
@gvreddy04 I've updated my PR. I have fixed it by getting rid of the background color and using indention instead. For example: @gvreddy04 could you tell why the I don't see any benefit in using the |
Hi @gvreddy04, I gave this one another shot. Now I know why you'll need to use the I've added a new commit with brings back the Feel free to give me feedback or suggestions :) |
@gvreddy04 would be nice to have this one in the next version. Right now I use a custom copy of the component in my app in production. |
Did you find some tiem to come back to this? I really need this in our apps. |
@gvreddy04 I've updated this PR with the removal of the |
@MarvinKlein1508 Taken care in #605. |
@gvreddy04 thanks for the info. Could you tell my why you are using this Id and ParentId apporach? Why not have a List within the |
ping @gvreddy04 |
@MarvinKlein1508 Sidebar implemented with the Id and ParentId approach. First, we want to maintain the same consistency for Sidebar2 so that people can switch without any extra code changes. Second, some people might persist NavItem (or user privileges) in the database. This is where the implementation began. Performance-wise, we will revisit Sidebar and Sidebar2 in the next release. You raised a concern with this approach. I'm not opposed to your feedback. I'll see about providing an option to input data in two formats. I'll create a Feedback issue; please add your proposal and its benefits. Also, mention in your case whether you are providing dynamic or static data. Our intention is to help developers and make them more productive with the ease of use of our components. I always appreciate your contributions in all ways, including pull requests and answering others' questions. Thank you! |
@MarvinKlein1508 Feedback: Sidebar & Sidebar2 Components #620 |
This PR reworks the implementation of the sidebar to be more easy to setup.
In addition it adds unlimited levels of navitems which makes #336 obsolete.
I'Ve updated all demos and got rid of the delegate.
In addition this implemenation renders much faster compared to the old one.
Feel free to suggets changes.