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

Figure identifiers must be lowercase, no spaces, etc, for scroll-to-lightbox-slide to work #869

Open
1 task done
cbutcosk opened this issue Oct 16, 2023 · 6 comments
Open
1 task done
Assignees
Labels
enhancement Proposed new feature. There are no guarantees a suggested enhancement will be included in Quire. quire-11ty This is an issue with the quire-11ty package status:backlog Issue is a lower priority but needs to eventually be addressed

Comments

@cbutcosk
Copy link
Collaborator

Before proceeding, make sure there isn’t an existing issue for this bug.

  • I have searched the existing issues and determined this is a new bug.

Expected Behavior

Adding images to figures.yaml that have uppercase letters, periods, or underscores I expect clicking on an image in the essay to still bring up that figure's slide in the lightbox modal.

Or, if that's not going to work I expect it to break on build and tell me why. And I expect any constraints on figure identifiers to be documented. :)

Actual Behavior

The figures are processed fine and appear in the essay but clicking on a figure brings you to the start of the lightbox rather than the slide of the figure you clicked. The only way I was able to get the spec data below to build was to replace figure identifiers' uppercase chars with lowercase and all periods / . with dashes (underscores as replacements did not work).

Note that the quire docs use a lot of figures with '.' in them!

Steps to Reproduce

With this drawn-from-real data:

  - id: "XXXX.1111.22"
    src: figures/XXXX_1111_22_FPO.jpg
    caption: "<cite>Work Title</cite>, Work Cataloguing Details. Various dimensions."
    credit: "ZZZZ © PHOTO CREDIT TK."
    zoom: true

  - id: "YYY-33333"
    src: figures/YYY-33333.jpg
    caption: "<cite>Another Work Title</cite>, 1980. Detailed cataloguing, Various dimensions."
    credit: "ZZZZ © PHOTO CREDIT TK."
    zoom: true

Version Numbers

[CLI] Command 'info' called with options { debug: true }
[test-latest]
quire-cli 1.0.0-rc.10
quire-11ty 1.0.0-rc.14
starter https://github.com/thegetty/[email protected]
[System]
quire-cli 1.0.0-rc.10
node v18.17.0
npm 9.6.7
os Darwin 22.6.0

Web Browser

Platform: macOS, chrome: latest, safari: 17.0

Relevant Terminal/Shell Output

No response

Supporting Information

No response

@cbutcosk cbutcosk added the status:triage needed Issue needs to be triaged by the Quire team. This label is automatically applied to new issues. label Oct 16, 2023
@Erin-Cecele
Copy link
Collaborator

Thank you, @cbutcosk, for bringing this to our attention and noting the discrepancy in the documentation. We'll take a closer look and keep you updated.

@Erin-Cecele
Copy link
Collaborator

Erin-Cecele commented Nov 30, 2023

Hi @cbutcosk I was able to recreate these errors via testing and actually couldn't even get the preview to work. I noticed the documentation is particularly wrong, as it features examples using periods! So it appears ids have become more sensitive since we switched from Hugo to 11ty. This one slid past us, as we only ever use dashes in ids at Getty. Thank you for finding and reporting!

I will absolutely update the documentation to clarify. Can you see any reason why we should push to make ids less sensitive or is updating the documentation sufficient to address this issue?

@Erin-Cecele Erin-Cecele added quire-11ty This is an issue with the quire-11ty package status:in progress Issue is under development and removed status:triage needed Issue needs to be triaged by the Quire team. This label is automatically applied to new issues. labels Nov 30, 2023
@cbutcosk
Copy link
Collaborator Author

cbutcosk commented Dec 5, 2023

@Erin-Cecele Great, thanks for updating! You can close this issue for now I think--although updating the code to be more flexible around figure id constraints would be a nice to have ("structure your ids so they're document query selectors" is... weird) it's downstream of some other gotta-fix-that's in the quire runtime.

@Erin-Cecele
Copy link
Collaborator

Thanks @cbutcosk. Ideally, Quire's handling of ids would be analogous to the rules for id values in HTML: ID and NAME tokens must begin with a letter ([A-Za-z]) and may be followed by any number of letters, digits ([0-9]), hyphens ("-"), underscores ("_"), colons (":"), and periods ("."). But yes this is a bit downstream from some of the more pressing fixes on the docket.

As a side note, we also discovered that Quire is converting any id used (one coming from figures.yaml or just an id that was added to a random element on the page) to be only lowercase; and underscores, colons and periods are converted to hyphens. This appears to be a separate but most likely related issue, so stay tuned as, we may be opening another ticket for that.

@Erin-Cecele Erin-Cecele added status:backlog Issue is a lower priority but needs to eventually be addressed enhancement Proposed new feature. There are no guarantees a suggested enhancement will be included in Quire. and removed status:in progress Issue is under development labels Dec 6, 2023
@Erin-Cecele
Copy link
Collaborator

We are backlogging this issue as we eventually need to determine what subset of valid ids will work across JavaScript, HTML, and CSS.

@cbutcosk
Copy link
Collaborator Author

cbutcosk commented Dec 7, 2023

@Erin-Cecele Hah just the issue I mean. On the one hand whatever bugs that relaxing the slugifying of ids introduces need fixed and on the other hand / as a result the js runtime needs some event handling cleanup and more pub data intelligence so it does less direct querySelector-ing of params.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Proposed new feature. There are no guarantees a suggested enhancement will be included in Quire. quire-11ty This is an issue with the quire-11ty package status:backlog Issue is a lower priority but needs to eventually be addressed
Projects
None yet
Development

No branches or pull requests

2 participants