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

Feature: Refactor lightbox to generate slides dynamically and refine web componentry #952

Merged
merged 23 commits into from
Sep 25, 2024

Conversation

cbutcosk
Copy link
Collaborator

@cbutcosk cbutcosk commented Sep 5, 2024

This PR (along with thegetty/quire-starter-default#45) refactor the q-lightbox element:

  • Improves subcomponent composition with four named slot elements for data, ui, slides, and styles
  • Compiles lightbox styles and inserts them in the component's style slot at publication build-time
  • Generates slides dynamically at component startup from JSON
  • Refactors image-sequence from 11ty-webc to Lit, renamed q-image-sequence


const { assetDir } = eleventyConfig.globalData.config.figures

return async function(...args) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return async function(...args) {
/**
* lightboxData shortcode component function
* @param {Object} Figures data object
* @return {string} HTML script element with the JSON-serialized payload
*/
return async function(...args) {

Comment on lines 84 to 88
return html`<script type="application/json"
class="q-lightbox-data"
slot="data">
${jsonData}
</script>`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return html`<script type="application/json"
class="q-lightbox-data"
slot="data">
${jsonData}
</script>`
return html`
<script type="application/json" class="q-lightbox-data" slot="data">
${jsonData}
</script>
`

Comment on lines 159 to 171
const { id,
mediaType,
figureElementContent,
annotationsElementContent,
aspectRatio,
isVideo,
isAudio,
label,
labelHtml,
caption,
captionHtml,
credit,
creditHtml } = figure
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const { id,
mediaType,
figureElementContent,
annotationsElementContent,
aspectRatio,
isVideo,
isAudio,
label,
labelHtml,
caption,
captionHtml,
credit,
creditHtml } = figure
const {
id,
mediaType,
figureElementContent,
annotationsElementContent,
aspectRatio,
isVideo,
isAudio,
label,
labelHtml,
caption,
captionHtml,
credit,
creditHtml
} = figure

Comment on lines 10 to 12
* Returns an HTML script element with the JSON-serialized payload
*
* */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Returns an HTML script element with the JSON-serialized payload
*
* */
* @returns HTML script element with the JSON-serialized payload
*/

mphstudios
mphstudios previously approved these changes Sep 23, 2024
Copy link
Member

@mphstudios mphstudios left a comment

Choose a reason for hiding this comment

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

@cbutcosk this looks great, I made a few linting comments.

@cbutcosk cbutcosk dismissed mphstudios’s stale review September 23, 2024 18:58

The merge-base changed after approval.

@cbutcosk cbutcosk merged commit 74bb637 into main Sep 25, 2024
@cbutcosk cbutcosk deleted the feature/lightbox-slides-from-data branch September 25, 2024 17:35
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