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

Core rewrite #2858

Merged
merged 154 commits into from
Oct 24, 2023
Merged

Core rewrite #2858

merged 154 commits into from
Oct 24, 2023

Conversation

bjoluc
Copy link
Member

@bjoluc bjoluc commented Nov 9, 2022

Hey @jspsych/core,
my initial v8 work is ready 🎉 It is fully backwards compatible except for the changes mentioned in the changeset. The remaining TODOs for this PR are:

  • Update plugin unit tests and make the CI pass (working on that now)
  • Update the docs according to the changeset – I didn't touch the markdown docs yet, and I'd love some help with that 📖 Maybe we should also start writing a migration guide already?
  • There are some plugin parameters that are marked as array, but are intended to accept non-array values too. The new implementation throws an error in that case. What should we do? Update the plugins? Just remove the error? Find some other solution? I'm happy about your ideas!

Once this PR is ready, we can merge it into v8 and implement new features in separate PRs against v8 so things keep trackable. Excited to move this forward!

It was based on a misconception on my end
@jodeleeuw
Copy link
Member

Hey @jspsych/core I've added the documentation updates here that are related to the core changes. I still need to update the docs for the button plugins. I'm gonna make some new examples for those, so it may take me a little bit longer.

@jodeleeuw
Copy link
Member

@bjoluc I'm wondering if the default flexbox layout for the button response plugins is flexible enough. One of the queries that pops up on the discussion forum periodically is about arranging the buttons into a grid. I wanted to make an example like that, but it's not straightforward to implement in the new button_html function.

A few options:

  1. We also expose a parameter for the container of the buttons, with flex as the default. Would allow people to use any layout option.
  2. We pick a few common layout options and parameterize them in the plugins.
  3. We leave it as is and show people how to use CSS to override the layout option.

What do you think?

@bjoluc
Copy link
Member Author

bjoluc commented Sep 25, 2023

@jodeleeuw Wonderful, thanks! As for the button response flexbox layout: I think achieving different layouts with display: flex in plain CSS is simple enough to decide against (2) and retain full CSS flexibility. Then the remaining choices would be a (1)-ish approach allowing to override (the entire, I guess?) container inline CSS, or just (3). I'm fine with both. I don't know how common custom layout needs are, i.e. if (1) is justified.

@jodeleeuw
Copy link
Member

I get the sense from the discussion forum that arranging buttons in a grid is common enough that we should support it. Maybe I'm lacking imagination, but I can't think of a simple way to do that with display: flex, given that each button element is at the same level in the DOM when you use button_html functions. Is there something I'm missing?

@bjoluc
Copy link
Member Author

bjoluc commented Sep 25, 2023

but I can't think of a simple way to do that with display: flex

Argh, sorry, I was thinking of display: grid as a Flexbox concept which it is obviously not (although they share some properties)! If the single most common non-default layout is a grid, then I'm open to (2) too!

@jodeleeuw
Copy link
Member

I think this might be ready to merge into v8?

@bjoluc bjoluc marked this pull request as ready for review October 24, 2023 09:09
@bjoluc bjoluc merged commit 9926a79 into v8 Oct 24, 2023
2 checks passed
@bjoluc bjoluc deleted the core-rewrite branch October 24, 2023 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment