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-479: Adds the sbf_file_content extension #480

Open
wants to merge 8 commits into
base: 1.5.0
Choose a base branch
from
Open

Conversation

DiegoPino
Copy link
Member

@DiegoPino DiegoPino commented Nov 13, 2024

See #479

works like this

{{ sbf_file_content("uuid_of_the_node", "uuid_of_the_file", "thename.jsonld")|raw }}

Will return the content of a file if found/matches one of the mimetypes defined as valid for our metadata displays, now defined as constants at

CONST ALLOWED_MIMETYPES = [
    'text/html' => 'HTML',
    'application/json' => 'JSON',
    'application/ld+json' => 'JSON-LD',
    'application/xml' => 'XML',
    'text/plain' => 'TEXT',
    'text/turtle' => 'RDF/TURTLE',
    'text/csv' => 'CSV',
  ];

an error message in case of exception or an empty string if the file does not exist, the user has no permission, is not of an allowed type, etc (avoiding as much as possible errors)

Also fixes #483

@alliomeria more tomorrow bc this solves VARIOUS use cases

works like this
sbf_file_content("uuid_of_the_node", "uuid_of_the_file", "thename.jsonld");

Will return the content of a file if found/matches one of the mimetypes defined as valid for our metadata displays, now defined as constants at

CONST ALLOWED_MIMETYPES = [
    'text/html' => 'HTML',
    'application/json' => 'JSON',
    'application/ld+json' => 'JSON-LD',
    'application/xml' => 'XML',
    'text/text' => 'TEXT',
    'text/turtle' => 'RDF/TURTLE',
    'text/csv' => 'CSV',
  ];
an error message in case of exception or an empty string if the file does not exist, the user has no permission, is not of an allowed  type, etc (avoiding as much as possible errors)

@alliomeria i am truly smartz! (and modest too!)
@DiegoPino DiegoPino added enhancement New feature or request Twig Twig template processing metadata Meta(l) data IIIF Specs/Manifests/Implementations Twig Extension Make those flowers reach the sun Drupal 10 Upgrade economy labels Nov 13, 2024
@DiegoPino DiegoPino added this to the 1.5.0 milestone Nov 13, 2024
@DiegoPino DiegoPino self-assigned this Nov 13, 2024
Basically keep doing that/delaying the event until (if ever) we have an event listener.
Also attach the event listener only to the Body. We need it only once. Not more.
Also updates them. Probably missing some "side" effects of this
@patdunlavey
Copy link
Collaborator

Hi Diego, I did some testing of this commit. Some things I noticed, though I'm not sure if they are new things (isn't that always the problem with requests to "test this"?!). In this table, "no" means I think this is or may be a problem, and "yes" means that I think it's correct.

Test Mirador 3 from CDN Mirador 4 from CDN Custom Archipelago Mirador with Plugins (Mirador 4 from CDN selected) Custom Archipelago Mirador with Plugins (Mirador 3 from CDN selected)
Display mode defaults to "single" (1-up) ("no" means that it defaults to "book") no no yes yes
In "book" mode, page hash in the url is updated when the active page changes no no no no
In "single" display mode, page hash in the url is updated when the active page changes yes yes no no
Manually adding "#page/N" to the url and pressing enter causes the browser page to reload with Mirador showing the correct page no (console shows a javascript error) no (console shows a javascript error) no (console shows a javascript error) no (console shows a javascript error)
Pressing enter a second time reloads the object with the Mirador viewer showing the correct page (both single and book display modes) yes yes yes yes

@patdunlavey
Copy link
Collaborator

I'm sorry that I did not include testing search, since I don't have that set up in this project.

@DiegoPino
Copy link
Member Author

@patdunlavey sorry I don't understand the test matrix. I don't see the console errors, and also 1up/2up is not in what this delivers. Very confused. Could you provide more info? Thanks a lot

@patdunlavey
Copy link
Collaborator

Thanks Diego. Here's the console error that I'm seeing (Linux Chrome) when I add/edit the #page/NN hash to the url and submit (press <enter>):
image
Pressing <enter> a second time triggers the page reload as I would expect.
I see the same error and behavior in Linux Firefox.

@patdunlavey
Copy link
Collaborator

The other important thing that I'm seeing is that, when "Custom Archipelago Mirador with Plugins" is selected, I'm seeing Mirador always load in "book" (2-up) display mode. If that's not something you're seeing, that will be useful to know. (I can't think of anything I might be doing to inadvertently change the default display mode.)

@DiegoPino
Copy link
Member Author

@patdunlavey but that error is not at the Archipelago level. There is a different JS in your page (form.js) that is triggering handleFragmentLinkClickorHashChange (look at your debug stack) that is failing bc actually those IAbookreader hashes are not valid Document Element IDs at all. Not sure why you have the form.js loaded on a landing page though

@DiegoPino
Copy link
Member Author

For the 1up/2up thing I will say "we won't implement". There are many reasons why a user will not want an enduser/link to mess up with the display (selected) based on the actual IIIF Manifest. The manifest (and you opened an ISSUE which I solved) can and will define the display mode using the IIIF specs. And with the Mirador override JSON we could even disable (and some users have requested that) the Display Mode change on the UI, so allowing a link to trigger that bypassing the choices is a bad idea

@patdunlavey
Copy link
Collaborator

I suspect form.js loads because we have the search form displayed on all pages (in the page header region).

@DiegoPino
Copy link
Member Author

With form.js, The issue there is an order of operations problem with Drupal. I can not tell Drupal in which order their internal libraries will be loaded. Happy though to not use IAbookreaders pattern at all, but then we (maybe you?) will have to make a wrapper code that uses the "new-valid element id" hash and transform/read on the fly at the IAbookreader level, bc we need to unify this approach
(notice that iabookreader might be loading before form.js)
Or you can override form.js (will take a pull) to not blindly pass the hash into JQUERY ... to avoid the error

@patdunlavey
Copy link
Collaborator

Does this look to you like it might be the (best) solution for the form.js thing? https://www.drupal.org/project/drupal/issues/2395065. I will test the patch and see if it solves the problem.

@patdunlavey
Copy link
Collaborator

I tested the patch and, while it makes the console error go away, it seems to simply swallow it. The page still doesn't load.

@patdunlavey
Copy link
Collaborator

patdunlavey commented Nov 18, 2024

Regarding the default 2-up thing I'm seeing, it seems to be consistent with "behavior": ["paged"] in the iiif v3 manifest. I tested changing it to "individuals", and it does indeed result in defaulting to single pages.
But it's curious that...

  1. "paged" is the default in archipelago-deployment, and...
  2. Despite this, Mirador defaults to single page display when the "Custom Archipelago Mirador with Plugins" option is selected.

Am I misunderstanding the purpose of the "behavior" attribute in the iiif manifest?

@DiegoPino
Copy link
Member Author

@patdunlavey not sure why that version of mirador (with the plugin) behaves like that.
Could you try commenting this here?

if (drupalSettings.format_strawberryfield.mirador[element_id]['custom_js'] == true) {
$options.window = {
workspaceControlPanel: {
enabled: false
},
allowClose: false,
imageToolsEnabled: true,
imageToolsOpen: true,
views: [
{ key: 'single', behaviors: [null, 'individuals'] },
{ key: 'book', behaviors: [null, 'paged'] },
{ key: 'scroll', behaviors: ['continuous'] },
{ key: 'gallery' },
],
};
$options.windows[0].workspaceControlPanel = {
enabled: false
};
$options.windows[0].workspace = {
isWorkspaceAddVisible: false,
allowNewWindows: true,
};
}

Maybe the differentiated defaults we have for that JS are overriding the internal?

Let me know

@DiegoPino
Copy link
Member Author

I tested the patch and, while it makes the console error go away, it seems to simply swallow it. The page still doesn't load.
Any messages in your logs?

@DiegoPino
Copy link
Member Author

@patdunlavey Will check if the way we alter the browser history is basically telling the caching of the browser that a reload is the same page or not. I can't reproduce the error you see and i get no errors in js at all, but will give this a try again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Drupal 10 Upgrade economy enhancement New feature or request IIIF Specs/Manifests/Implementations metadata Meta(l) data Twig Extension Make those flowers reach the sun Twig Twig template processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants