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

Consolidates data dialog components #17802

Merged
merged 47 commits into from
Apr 8, 2024

Conversation

guerler
Copy link
Contributor

@guerler guerler commented Mar 21, 2024

This PR consolidates data dialog components and converts them to composition api and type script. This is a preparation for upcoming changes to the data dialog functionality. Additionally this PR decomposes some of the nested abstraction layers. Moving forward it should be easier for us to make additions or customize the data dialog for use in different contexts.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@guerler guerler added area/UI-UX kind/refactoring cleanup or refactoring of existing code, no functional changes labels Mar 21, 2024
@guerler guerler added this to the 24.1 milestone Mar 21, 2024
@jmchilton
Copy link
Member

I'd really like the ability to extend the Markdown editor components with the ability to customize the form for each directive you insert with added option (e.g. add an optional footer, select a size from a dropdown, etc..). Mostly these are all just these selection dialogs currently. I hope you can keep this use case in mind as you refactor these!

@guerler
Copy link
Contributor Author

guerler commented Mar 28, 2024

@jmchilton absolutely, the goal of this refactoring is to simplify the overall structure while at the same allowing for more flexibility. As we go along we can easily add additional slots to extend or replace the footer or other components as necessary. I'd be happy to help with that.

@guerler guerler marked this pull request as ready for review March 28, 2024 11:16
Copy link
Member

@jmchilton jmchilton left a comment

Choose a reason for hiding this comment

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

My gut says I don't think that slots is the right approach - the Markdown pickers are not data pickers with extra options they are a set of options that include data as one. Conceptually not a huge difference but I think it guides one to a different approach. The right approach would be to extract the stuff inside the modal as a component that could be included in other modals not making the existing modal more complex and having to deal with the complexity of the existing modal. I think that sort of leads us down the road of Forms and Grids - where you think there should be a big hyper-parameterizable mega widget and I think we should have smaller composable ones even if that means a lot more duplication. You're a brilliant developer though and I understand your vision and I know you always find a way to make it work and work well. I'm happy to try it your way - especially if you were willing to do the work on a few of the markdown components to outline what it should look like.

@jmchilton jmchilton merged commit 38c1a27 into galaxyproject:dev Apr 8, 2024
31 checks passed
@guerler guerler deleted the revise_datadialogs branch April 9, 2024 16:23
@guerler
Copy link
Contributor Author

guerler commented Apr 9, 2024

@jmchilton I will definitely put the work in to adjust this as needed as we go along, while avoiding over engineered frameworks with overly complex components and a huge number of props. I really appreciate it. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/UI-UX kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants