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

Add button_layout parameter to support CSS grid #3152

Merged
merged 12 commits into from
Oct 19, 2023

Conversation

jodeleeuw
Copy link
Member

This PR is trying to resolve the button layout issue in #2858.

It adds a button_layout parameter to the html-button-response plugin, and two corresponding parameters for button_rows and button_cols.

I think the grid layout is an improvement over the flex layout, so I also decided to make it the default. But flex is still supported.

If these changes look reasonable to @jspsych/core I'll update the other button plugins as well.

@changeset-bot
Copy link

changeset-bot bot commented Oct 16, 2023

🦋 Changeset detected

Latest commit: 45e0d65

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@jspsych/plugin-html-button-response Major
jspsych Patch

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

@jodeleeuw
Copy link
Member Author

(I'll update the tests if the general approach seems right)

@becky-gilbert
Copy link
Collaborator

Agh, sorry Josh - for some reason I'm not receiving the jspsych/core notifications so I missed your question about this on #2858.

This is actually my preferred solution. I think this will cover the most non-standard-layout use cases. If anyone is doing something really specific/unusual with the layout, we can point them in the direction of overriding the CSS.

Copy link
Collaborator

@becky-gilbert becky-gilbert left a comment

Choose a reason for hiding this comment

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

Pending changes to failing tests 🙂

@jodeleeuw jodeleeuw marked this pull request as draft October 18, 2023 13:31
Copy link
Member

@bjoluc bjoluc left a comment

Choose a reason for hiding this comment

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

Looks very useful in general 👌

packages/plugin-html-button-response/src/index.ts Outdated Show resolved Hide resolved
examples/jspsych-html-button-response.html Show resolved Hide resolved
packages/jspsych/src/index.scss Outdated Show resolved Hide resolved
@jodeleeuw
Copy link
Member Author

jodeleeuw commented Oct 18, 2023

@bjoluc the failing tests are because of Jest: Couldn't locate all inline snapshots. on my local build. Is this a configuration problem on my end?

EDIT: clarification that this is when I try to update the snapshots using npm run test button -- -u

@bjoluc
Copy link
Member

bjoluc commented Oct 18, 2023

Is this a configuration problem on my end?

Nope, Jest tries to find the right position to update each inline snapshot at and doesn't succeed for some reason (Surcrase sourcemap issues?). I was erroneously hoping this would be fixed in a feature version without me looking into it. We have two options: Use regular snapshots (the ones with external files) instead of inline snapshots, or update the snapshots manually for the time being. I didn't completely give up on this and I'm planning to look into it when I get a chance, so until now I've always just manually updated the inline snapshots according to failure messages. Feel free to decide for either approach.

@jodeleeuw jodeleeuw marked this pull request as ready for review October 19, 2023 13:26
@jodeleeuw jodeleeuw merged commit bb3d82a into core-rewrite Oct 19, 2023
@bjoluc bjoluc deleted the button-plugin-update branch November 8, 2023 16: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.

3 participants