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

ISSUE-237: Move ADO Tools into this module, make JMESPATH a subsection + add a view for listing children #238

Closed
wants to merge 18 commits into from

Conversation

aksm
Copy link
Contributor

@aksm aksm commented Nov 4, 2022

Starts to resolve #237.

Move existing ADO Tools menu and JMESPATH codemirror library and update any references. This just moves everything over from strawberry_runners while retaining the exact same functionality (tested).

esmero/strawberry_runners#63 should be merged before merging this.

@aksm aksm self-assigned this Nov 4, 2022
@aksm aksm marked this pull request as ready for review November 16, 2022 16:26
@aksm aksm requested a review from DiegoPino November 16, 2022 16:29
@DiegoPino
Copy link
Member

@aksm hi. This is bringing commits that don't belong here. Are those OK?
e.g src/Controller/StrawberryfieldFlavorDatasourceSearchController.php

Can you please double check/rebase and cleanup? Thanks

@aksm
Copy link
Contributor Author

aksm commented Nov 17, 2022

@DiegoPino I had merged in recent changes up to that point. Should I merge in the further changes since?

@aksm hi. This is bringing commits that don't belong here. Are those OK? e.g src/Controller/StrawberryfieldFlavorDatasourceSearchController.php

Can you please double check/rebase and cleanup? Thanks

@DiegoPino
Copy link
Member

@aksm normally you don't need to merge other changes. You fetch a branch, create a new one from that one and you only modify what you need too. Only reason to merge other changes is when you are touching those files. Here you can rebase to 1.1.0. Or are these changes in 1.0.0 not present in 1.1.0?

@aksm
Copy link
Contributor Author

aksm commented Nov 17, 2022

Ah ok, these were just recent changes from 1.1.0. I'll roll back and keep just my commit.

Copy link
Member

@DiegoPino DiegoPino left a comment

Choose a reason for hiding this comment

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

Small changes. Sorry, those were wrong from the old code!

Thanks for your patience (excellent service!)

Copy link
Member

@DiegoPino DiegoPino left a comment

Choose a reason for hiding this comment

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

Still a few unresolved issues

src/Form/StrawberryfieldToolsForm.php Outdated Show resolved Hide resolved
@aksm
Copy link
Contributor Author

aksm commented Mar 20, 2023

@DiegoPino Will make a separate PR for the views needed for this, but here is what I used for developing/testing:

ado_tools_children_views.zip

@aksm
Copy link
Contributor Author

aksm commented May 12, 2023

Hi @DiegoPino, except for the views (see above comment), I think this is ready for review.

@DiegoPino
Copy link
Member

@aksm thanks. Will review during the day!

@DiegoPino
Copy link
Member

DiegoPino commented May 12, 2023

@aksm regarding the default views.
I would explore using the config/install folder of this module and see also this function search_api_solr_update_helper_install_configs (as a reference) which should allow us to deploy selectively the new views via an update hook (v/s having to uninstall/reinstall the module)

See also https://www.drupal.org/docs/creating-modules/include-default-configuration-in-your-drupal-module

@aksm
Copy link
Contributor Author

aksm commented May 14, 2023

@DiegoPino I basically copied the function you referenced above as is in order to push the commits in by EOD Friday (which failed anyway because I was in a detached head state and ended up coming back later in the evening here: 00ec5c9), and it seemed to work when I tested, but I'm glad to keep on it if you think there's more needed or if anything's missing. Either way, I'm sure the views will need more tweaking.

@DiegoPino
Copy link
Member

@aksm thanks. I will try ti get back to this early morning Monday. Have a full day of IIIF (even if half Sunday is over) ahead so might better look at this with fresh eyes. Hugs and thanks again

@aksm aksm removed their assignment Sep 29, 2023
@aksm aksm closed this by deleting the head repository Oct 2, 2023
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.

2 participants