New plugin: Sketchpad #2258
New plugin: Sketchpad #2258
Conversation
🦋 Changeset detectedLatest commit: 053a8a9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
I think this is basically ready if anyone (@becky-gilbert @bjoluc @kushinm @chrisbrickhouse ?) wants to take a look! The testing suite isn't super extensive yet, and testing will be limited because of limited support for canvas drawing in jsdom. The only features I didn't implement that were brought up in #2196 are a handler to save the image files to a server directly and path smoothing. I figured both of these are reasonable to target for |
| href="https://unpkg.com/jspsych@7.0.0/css/jspsych.css" | ||
| /> | ||
| <style> | ||
| .jspsych-btn { |
There was a problem hiding this comment.
Side note: I wonder if this should be added to the jsPsych package's CSS file? For most trial types it doesn't make a difference, but for survey trials the page content is often larger than the window height, which means the navigation buttons are positioned at the very bottom edge of the page.
There was a problem hiding this comment.
Maybe the solution is to add top/bottom padding to .jspsych-content?
Interesting Q whether that then becomes a "major" release!
There was a problem hiding this comment.
Yeah maybe that's a better solution.
I would hope it's not a major release! Definitely shouldn't break anything, right...?
There was a problem hiding this comment.
SemVer requires a major increment when there are incompatible API changes. The individual parts of the jsPsych default CSS aren't documented as part of the API; the only documented entry point is all or nothing. Functionally, the API is "either (1) don't use our defaults (2) use our defaults unmodified (3) use our defaults but overwrite as needed". That interface is still the same, so there is no requirement to increment the major version.
I'd argue for a patch increment. The intended bottom padding behavior was to inherit whatever prior styles said, and this happened to usually be 0 which causes problems for tall experiments. CSS is changed to fix this problem in a way that doesn't affect the API or programs not experiencing the problem. Sounds like a bug fix to me.
There was a problem hiding this comment.
I would hope it's not a major release! Definitely shouldn't break anything, right...?
I would hope so too, but it would apparently add a scrollbar in some experiments where I set children of .jspsych-content to 100vw/vh. I'd consider that breaking in these experiments (given that it can affect touch inputs on mobile devices). I also find the entries absolutely correct semver and pragmatically correct semver in the changesets dictionary interesting in this regard.
CSS is changed to fix this problem in a way that doesn't affect the API or programs not experiencing the problem
Unfortunately not in my case 🙁
That interface is still the same, so there is no requirement to increment the major version
I tend to disagree here: When a dependency is minor- or patch-updated, I expect my application to work as it previously did, unless the previous behavior was unintended. While the style of the root element is arguably not a part of the documented API, I think it is still very relevant since it has the potential to affect the layout of every trial in every plugin.
That being said, I would also agree with breaking the rules here, if we expect full-screen elements to be a rare edge case.
There was a problem hiding this comment.
it would apparently add a scrollbar in some experiments
That's actually very interesting, so thanks for that example. That seems like something that could be fixed by well-crafted CSS (see this stackoverflow post) and well-defined problems and use cases. That said I think we've gotten beyond the scope of this review, discussing future changes to the main CSS. At that point there may be enough changes that it can just be included in an already planned version bump.
|
Again, happy to make any changes myself - just let me know 😃 |
This will be the canvas drawing plugin. Decided not to call it
canvas-drawingbecause it might be confusing with other plugins likecanvas-button-responsein which the canvas refers to the stimulus, not the response mechanism.Closes #2196.