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

move Hint and Solution knowls to details/summary disclosure #941

Merged
merged 8 commits into from
Oct 30, 2023

Conversation

Alex-Jordan
Copy link
Contributor

This is certainly not ready for a merge. But read on.

This moves "Hint" and "Solution" away from knowls that need the knowl js, and instead uses the HTML5 native disclosure widget that is a details element with a summary child. The advantage of the move is that something fancy with js is now supported mostly with just HTML and CSS.

For now, there are a bunch of default bootstrap classes that get the styling started.

At minimum, it needs much better styling, and I think @drgrice1's expertise is needed here. The styling may need a level of bootstrap understanding that you have better than I do. Also since the current knows are styled using pg/htdocs/js/Knowls/knowl.css, I am wondering if some CSS file inside pg is where styling for this should happen (as opposed to in webwork2/.../math4.scss). And I'm unsure where that should be and if it would be a new (s)css file, how to introduce that.

There is a chevron next to each "Hint" or "Solution" and it is possible to animate that to rotate it as you open/close the item. And it is possible to have a little js to animate the expansion/retraction of the hidden content. Here is one page demonstrating an approach for both of these: https://christianoliff.com/blog/building-a-bootstrap-accordion-with-the-details/summary-html-tags/

Again since it may involve a new CSS file, and especially if it involves new js, I am wondering if @drgrice1 is interested in handling the cosmetic touches for this. PTX has moved to using details/summary for many of its (former) knowls, but does not use bootstrap.

@Alex-Jordan
Copy link
Contributor Author

I didn't look into it, but perhaps the knowlLink subroutine could be rebuilt to use details/summary and then maybe we could drop knowl.js.

@pstaabp
Copy link
Member

pstaabp commented Oct 13, 2023

I think is a start. A couple of things.

  1. I think it would be nice to animate the opening/closing. I looked at a few examples using CSS3 animations, so should be possible.
  2. I assume we will extend to other knowls items like the helpLink

@drgrice1
Copy link
Member

@Alex-Jordan: I added a pull request to this branch that adds animation of the button icon and body text expansion and contraction.

I am in favor of eliminating knowls completely. However, I do not think that this will work for knowls in general. The problem with knowls in general is how they are used, and that usage presupposes this approach from working. That is the placement of knowl links inline or in tables or lists. That won't work for this or accordions in general in any nice way. So, basically, the only way to eliminate knowls will be to rewrite all problems that use them to not place them in those settings, and insist that they are placed in a place that will work for display:block elements in general.

@Alex-Jordan
Copy link
Contributor Author

I tweaked some CSS and now in math4, it looks like:

Screenshot 2023-10-25 at 10 34 04 PM

I think it's OK, but there are two problems.

  • To do this, the color of the "Hint" and "Solution" relies on the theme. In particular, it uses var(--bs-primary) which is set over in the theme. Is it a concern for this PG thing to rely on a webwork2 style?
  • While this looks fine for blue, red, and green, it's bad for yellow. I'm not sure how to set the text color differently when the theme is the yellow theme.

Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

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

This looks mostly good. One contrast issue needs to be fixed though.

macros/core/PGbasicmacros.pl Outdated Show resolved Hide resolved
htdocs/js/Problem/problem.scss Outdated Show resolved Hide resolved
@Alex-Jordan
Copy link
Contributor Author

I applied the changes from this morning about the font color, font weight, and padding.

Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

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

Looks good.

Copy link
Member

@pstaabp pstaabp left a comment

Choose a reason for hiding this comment

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

Looks great.

@pstaabp pstaabp merged commit ca5295e into openwebwork:develop Oct 30, 2023
2 checks passed
drgrice1 added a commit to drgrice1/pg that referenced this pull request Oct 30, 2023
drgrice1 added a commit that referenced this pull request Oct 30, 2023
Revert local changes accidentally added to #941 in merge conflict resolution.
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