Problem/Motivation

Currently, there are for every theme setting two functions. One for the global and one for the user-specific value.
I think we could refactor the logic to make it more generic and easier in the future to add new settings.

Maybe we should keep the current setting names to keep it backwards compatible as possible.
Talking about

enable_darkmode: false
classic_toolbar: 'vertical'
CommentFileSizeAuthor
#4 3203078-4.patch13.83 KBchr.fritsch
#2 3203078.patch12.92 KBchr.fritsch

Issue fork gin-3203078

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chr.fritsch created an issue. See original summary.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
Status: Active » Needs review
FileSize
3203078.patch12.92 KB

Here is my suggestion. I added a service that handles the logic

Status: Needs review » Needs work

The last submitted patch, 2: 3203078.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
Status: Needs work » Needs review
FileSize
3203078-4.patch13.83 KB

Fixing the tests and CS issues

Status: Needs review » Needs work

The last submitted patch, 4: 3203078-4.patch, failed testing. View results

chr.fritsch opened merge request !21

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
Status: Needs work » Needs review

I think this is fine for now. I wouldn't change more. We can refactor more on other issues.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented

I removed the update hook, because themes are not able to declare those. So it was sadly useless.

saschaeggi’s picture

saschaeggi
he/him
CreditAttribution: saschaeggi as a volunteer commented
Status: Needs review » Needs work

See my questions in the PR :)

volkerk made their first commit to this issue’s fork.

volkerk’s picture

volkerk CreditAttribution: volkerk at Thunder commented
Status: Needs work » Needs review

saschaeggi closed merge request !21

saschaeggi reopened merge request !21

saschaeggi’s picture

saschaeggi
he/him
CreditAttribution: saschaeggi as a volunteer commented
Issue summary: View changes
Status: Needs review » Needs work

Added a Tugboat Live Preview Build to this issue.
Some stuff which needs work:
- Setting: "Users can override Gin settings" => should be disabled on a new installation
- Theme breaks with gin_toolbar for now (gin_toolbar would need refactoring as well I guess)

volkerk’s picture

volkerk CreditAttribution: volkerk at Thunder commented
Status: Needs work » Needs review
chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
Status: Needs review » Needs work

After installing in 3.x and then switching to the branch, the settings are still applied correctly, but in the settings form, the current values are not selected.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
Status: Needs work » Reviewed & tested by the community

Looks good to me now

saschaeggi’s picture

saschaeggi
he/him
CreditAttribution: saschaeggi as a volunteer commented
Status: Reviewed & tested by the community » Fixed

Thanks a lot @volkerk & @chr.fritsch for your heavy work on this 💙👏

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.