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

feat: support checked syndication targets #16

Merged
merged 2 commits into from
Nov 17, 2023

Conversation

paulrobertlloyd
Copy link
Contributor

@paulrobertlloyd paulrobertlloyd commented Nov 13, 2023

This PR attempts to add support for the checked property that can be returned in a query for an endpoint’s syndication targets.

I thought it might be as easy as adding the following to the individual checkboxes:

  m('label', [
    s.name,
    m('input', {
      type: 'checkbox',
+     checked: s.checked,
      onchange: e => updateSyndicateTo(e, s)
    })
  ])

While that worked, it prevented the checkboxes from being toggled. Also, updateSyndicateTo updates the mp-syndicate-to property when a checkbox is interacted with; with pre-checked checkboxes, a user might not interact with these form elements at all.

So instead I’m now calculating which syndication checkboxes have been checked on form submit.

However, perhaps given the way Mithril.js works – maybe updating and reseting the DOM on every event, I’m not sure – interacting with any other form element will reset the checkboxes back to their original state.

Hopefully there is enough here to begin reviewing this feature, and just a small change is needed to make this PR work as intended.

Copy link

netlify bot commented Nov 13, 2023

Deploy Preview for sprkls ready!

Name Link
🔨 Latest commit e306ce9
🔍 Latest deploy log https://app.netlify.com/sites/sprkls/deploys/6556e5c96c5d540008c657f6
😎 Deploy Preview https://deploy-preview-16--sprkls.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@paulrobertlloyd paulrobertlloyd marked this pull request as ready for review November 13, 2023 22:37
@benjifs
Copy link
Owner

benjifs commented Nov 17, 2023

Nice. I hadn't seen that proposal before so I appreciate the PR to get this added.

I just added a fix to get the checkbox to stop reverting to default state on any other input change so it should be good to go now.

@benjifs benjifs merged commit db206bb into benjifs:main Nov 17, 2023
3 checks passed
@paulrobertlloyd
Copy link
Contributor Author

🙌 Thanks for the quick review and merge.

@paulrobertlloyd paulrobertlloyd deleted the checked-syndicate-targets branch November 17, 2023 13:45
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