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

Remove configs. #19

Merged
merged 2 commits into from
May 31, 2023
Merged

Remove configs. #19

merged 2 commits into from
May 31, 2023

Conversation

rosiel
Copy link
Member

@rosiel rosiel commented May 29, 2023

What does this Pull Request do?

Removes optional configs in favour of putting them in the Starter Site.

What's new?

  • no longer add view modes, contexts, view displays, or EVA views.

  • Does this change add any new dependencies? No.

  • Does this change require any other modifications to be made to the repository
    (i.e. Regeneration activity, etc.)? No

  • Could this change impact execution of existing code? No

How should this be tested?

  • Follow the instructions to set up. Are they still satisfactory?

Documentation Status

  • Does this change existing behaviour that's currently documented? yes
  • Does this change require new pages or sections of documentation? no - included in PR
  • Who does this need to be documented for? - new site builders
  • Associated documentation pull request(s): ___ or documentation issue ___

Additional Notes:

Any additional information that you think would be helpful when reviewing this
PR.

Interested parties

Tag @alxp

@alxp
Copy link
Contributor

alxp commented May 30, 2023

I tested this on an existing site (not built with starter site) and uninstalling the movule, switching to the new branch, and re-enabling the module, causes the context that was enabled on the site to disappear.

I think removing the entity view displays and EVAs makes sense but I think we should leave the contexts and maybe also the view modes.

@rosiel
Copy link
Member Author

rosiel commented May 31, 2023

Yes, the purpose of this PR is for the mirador module, which shouldn't have opinions about how you make use of contexts or view modes, can have its config set up by the starter site.

I'm not in favour of having stub config in the mirador module, when all this kind of config makes sense with the rest of similar config in the starter site. Our testing framework is not rugged enough for us to maintain:

  • Doing this config as part of the starter site
  • and also having a "sensible default" for non-islandora users who are installing mirador alone (which is still not possible, with the requirement on islandora_iiif which is still a submodule of islandora). This default would also function as a starting point for those islandora users who are late (or early) to the party and want to install mirador on an existing site. We would have a hard time making it make sense without the rest of the islandora setup, and it would be impossible to maintain two possibly different ways of doing things.

@alxp
Copy link
Contributor

alxp commented May 31, 2023

OK after talking through this more I think we can just say 'contexts are a weird Islandora thing' so other modules that islandora may make use of don't need to define them themselves.

@wgilling wgilling merged commit 6438983 into 2.x May 31, 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.

3 participants