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

SDSS-1007: Stanford Basic install config fix for global footer variant #726

Closed
wants to merge 1 commit into from

Conversation

joegl
Copy link
Contributor

@joegl joegl commented Oct 18, 2023

READY FOR REVIEW

Summary

  • Switched to using #empty_option for Global Footer Variant, so default is now none. Didn't update install config setting -- this does that.

Review By (Date)

  • When does this need to be reviewed by?

Criticality

  • How critical is this PR on a 1-10 scale? Also see Severity Assessment.
  • E.g., it affects one site, or every site and product?

Urgency

  • How urgent is this? (Normal, High)

Review Tasks

Setup tasks and/or behavior to test

  1. Check out this branch
  2. Rebuild Cache and import config drush cr ; drush ci
  3. Navigate to...
  4. Verify...

Site Configuration Sync

  • Is there a config:export in this PR that changes the config sync directory?

Front End Validation

Backend / Functional Validation

Code

  • Are the naming conventions following our standards?
  • Does the code have sufficient inline comments?
  • Is there anything in this code that would be hidden or hard to discover through the UI?
  • Are there any code smells?
  • Are tests provided? eg (unit, behat, or codeception)

Code security

General

  • Is there anything included in this PR that is not related to the problem it is trying to solve?
  • Is the approach to the problem appropriate?

Affected Projects or Products

  • Does this PR impact any particular projects, products, or modules?

Associated Issues and/or People

  • JIRA ticket(s)
  • Other PRs
  • Any other contextual information that might be helpful (e.g., description of a bug that this PR fixes, new functionality that it adds, etc.)
  • Anyone who should be notified? (@mention them here)

Resources

@joegl joegl changed the base branch from 11.x to 8.x October 18, 2023 16:13
@joegl joegl changed the base branch from 8.x to 11.x October 18, 2023 16:14
@joegl
Copy link
Contributor Author

joegl commented Oct 18, 2023

Github is showing 4 files changed but it doesn't look like it's updated the diff since #724 was merged. This is a one liner.

@joegl joegl force-pushed the SDSS-1007--settings-fixup branch from b0156ec to d4a4b8d Compare October 18, 2023 16:16
@joegl joegl requested a review from pookmish October 18, 2023 16:17
@pookmish
Copy link
Contributor

it actually should be null due to the #empty_option now.
also there's a conflict.

@pookmish
Copy link
Contributor

You could also just remove the line entirely which makes it null

@@ -10,6 +10,7 @@ favicon:
logo:
use_default: true
brand_bar_variant: default
global_footer_variant: none
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this would be better. that makes it null

@pookmish pookmish force-pushed the 11.x branch 2 times, most recently from 17c82b4 to 209c154 Compare March 5, 2024 21:03
@pookmish
Copy link
Contributor

pookmish commented Sep 9, 2024

I'm going to close this. not having the value in the config results in a null value. That works better for the form and getting the value instead of checking == 'none'

@pookmish pookmish closed this Sep 9, 2024
@pookmish pookmish deleted the SDSS-1007--settings-fixup branch September 9, 2024 15:32
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.

2 participants