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

Clean up of the htdocs/themes directories. #2318

Merged
merged 2 commits into from
Feb 29, 2024

Conversation

drgrice1
Copy link
Member

Move gateway.scss and achievements.scss into htdocs/js subfolders. Rename the math4.js file to htdocs/js/System/system.js. Rename the math4.scss file to htdocs/js/System/system.scss. These files do not belong in the theme directories and should not be considered theme-able.

Where possible, the page specific code in the math4.js (now system.js) file has been moved into the page specific JavaScript file for that page.

The aria label hack that was in math4.js has been removed. It is clear that is not doing the correct thing, and it is probably less of a problem to have an aria label that reads "answer 1" even when there are no other answers than the numerous cases that it is changing aria labels to just "answer" when it shouldn't. One case is if there is only one text answer and there are other answers (radio buttons, checkboxes, etc) on the page. In this case the "answer 1" is changed to "answer" even when there is more than one answer on the page. Also in gateway quizzes the "problem n" prefix is removed if there is only one problem on the page, and it shouldn't be. Aria labels in general need to be rethought, but this JavaScript hack is just incorrect.

Update the README files in the themes directories a bit. More could be done here to give better directions on how to customize themes.

The name of the htdocs/js directory really needs to be changed. It is really the public "assets" location, and not just JavaScript. The htdocs/css directory should be removed as well.

Also change "npm install" to "npm ci" in the warnings that are issued if the static-assets.json files are not found.

Move the system.html.ep template out of the themes. That file is now the system layout instead. This file should really not be themable. If someone wants to mess with that file, they can deal with having locally modified files that show up as changed by git. If someone does not know what they are doing, they can really mess up things. If there is some other thing that is commonly wanted in the default system layout it can be added as an option.

So now the themes directories only have what is needed for theming, and theming for the most part is only about changing the color scheme. This is how it should be. Theming should not mean modify the entire system to make it be what you want.

There are several advantages with this pull request. It removes the need for so many symlinks from the math4 theme variants back to files in the math4 theme. It removes the need for the hackish htdocs/themes/layouts symlink that turns the themes directory into a layout directory. It removes the need for the themes directory to be a template path to begin with. The template files should not be in a publicly accessible location in any case.

@pstaabp pstaabp added the Update Wiki docs Significant changes to documentation in the Wiki are needed label Feb 13, 2024
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 good. I have a couple of customized themes and did remove the system.html.ep files from those directories.

Also added a label to update the wiki. I don't think the wiki will need much changes, but we should review that.

@drgrice1
Copy link
Member Author

With this pull request if you still have the old files (or links to them) in the themes directory for a customized theme, it won't matter. The files won't be looked for in those locations anymore. Most notably, this is true for the system.html.ep file. The themes directories are not template directories at all anymore.

@pstaabp
Copy link
Member

pstaabp commented Feb 13, 2024

Separate from this PR, but wondering if we should get rid of the 'math4' language in the theme. I know there were other themes before, but I would guess any newish users don't know anything but math4.

@drgrice1
Copy link
Member Author

Yeah, I also don't like the mathN naming. I didn't go to the extent of renaming it yet though.

@drgrice1 drgrice1 force-pushed the themes-dir-cleanup branch 2 times, most recently from fb518d7 to 6e9c296 Compare February 14, 2024 23:16
@drgrice1 drgrice1 force-pushed the themes-dir-cleanup branch 2 times, most recently from 5fce656 to d65be98 Compare February 25, 2024 12:12
Move gateway.scss and achievements.scss into htdocs/js subfolders.
Rename the math4.js file to htdocs/js/System/system.js.  Rename the
math4.scss file to htdocs/js/System/system.scss.  These files do not
belong in the theme directories and should not be considered theme-able.

Where possible, the page specific code in the math4.js (now system.js)
file has been moved into the page specific JavaScript file for that
page.

The aria label hack that was in math4.js has been removed.  It is clear
that is not doing the correct thing, and it is probably less of a
problem to have an aria label that reads "answer 1" even when there are
no other answers than the numerous cases that it is changing aria labels
to just "answer" when it shouldn't.  One case is if there is only one
text answer and there are other answers (radio buttons, checkboxes, etc)
on the page.  In this case the "answer 1" is changed to "answer" even
when there is more than one answer on the page.  Also in gateway quizzes
the "problem n" prefix is removed if there is only one problem on the
page, and it shouldn't be.  Aria labels in general need to be rethought,
but this JavaScript hack is just incorrect.

Update the README files in the themes directories a bit.  More could be
done here to give better directions on how to customize themes.

The name of the htdocs/js directory really needs to be changed.  It is
really the public "assets" location, and not just JavaScript.  The
htdocs/css directory should be removed as well.

Also change "npm install" to "npm ci" in the warnings that are issued if
the `static-assets.json` files are not found.
That file is now the system layout instead.  This file should really not
be themable.  If someone wants to mess with that file, they can deal
with having locally modified files that show up as changed by git.  If
someone does not know what they are doing, they can really mess up
things.

If there is some other thing that is commonly wanted in the default
system layout it can be added as an option.

So now the themes directories only have what is needed for theming, and
theming for the most part is only about changing the color scheme.  This
is how it should be.  Theming should not mean modify the entire system
to make it be what you want.

There are several advantages with this pull request.  It removes the
need for so many symlinks from the math4 theme variants back to files in
the math4 theme.  It removes the need for the hackish
`htdocs/themes/layouts` symlink that turns the themes directory into a
layout directory.  It removes the need for the themes directory to be a
template path to begin with.
@Alex-Jordan
Copy link
Contributor

Tested by checking out the branch and playing with theme settings. Not a thorough test, but I think it is good enough.

@Alex-Jordan Alex-Jordan merged commit 9c464b1 into openwebwork:develop Feb 29, 2024
2 checks passed
@drgrice1 drgrice1 deleted the themes-dir-cleanup branch February 29, 2024 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Update Wiki docs Significant changes to documentation in the Wiki are needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants