dspace-angular icon indicating copy to clipboard operation
dspace-angular copied to clipboard

Replace original selector when making a component themeable

Open ybnd opened this issue 1 year ago • 5 comments

Describe the bug Our current convention to enable theming for individual components is to 0. For a component Something with the selector ds-something

  1. Create a ThemedComponent<Something> wrapper, with the selector ds-themed-something
  2. Replace all usages of the original selector with the selector of the new wrapper

This convention can result in a lot of overhead, especially when upgrading existing customizations and themes to a more recent DSpace version

  • We essentially need to revisit step (2) for every component that has been made themeable upstream
  • It's easy to forget this, and the app will just keep working
  • ...but things will be inconsistent, especially if we start theming these newly themeable components

Proposed behavior We could easily circumvent this entire class of customization problems by adopting a different convention when making components themeable:

  1. Create a ThemedComponent wrapper for the component, with the same original selector, ds-something
  2. Replace the selector of the unthemed component with ds-base-something
  3. When creating overriding the component in a theme, use the selector ds-themed-something
    • Currently, unthemed and themed components use the same selector. This is possible because these selectors are never used in templates directly
    • If we use the original selector for the ThemedComponent wrapper, we cannot use it for themed components declared in the EagerThemeModule

In this way, all usages of the original unthemed component can stay as-is, and they will be automatically redirected to the themed wrapper without any changes.

Migration If we choose to migrate to this new approach, we'll need to

  • Clearly indicate this change in the wiki and changelog.
  • Apply the new convention to all existing themed components. I've done this in the past before, as a proof-of-concept, and it was pretty easy to do with a few regex search/replace operations

ybnd avatar Mar 01 '24 11:03 ybnd

Additional thoughts:

  • For this to work, we'll also need to update the selectors on component overrides in the actual themes
  • This will be reflected in the DOM, making it easier to debug which theme ends up being used

ybnd avatar Mar 01 '24 12:03 ybnd

Made a proof-of-concept branch by running the ZSH script below and cleaning up a few edge cases where the filenames did not match up correctly

https://github.com/ybnd/dspace-angular/tree/poc-reverse-themed-component-selector-convenction

# Requirements: https://github.com/BurntSushi/ripgrep
# Navigate to a DSpace Angular project before running

def rg-in-place() {
    FILE="${@[$#]}"
    TEMP="rg-in-place-temp"

    rg --passthru $@ > $TEMP && mv $TEMP $FILE

    if [[ -f $TEMP ]]; then
        rm $TEMP
    fi
}

echo "Updating component selectors"
for wrapper in $(rg -g 'themed-*.ts' -l "selector: ?'ds-themed"); do
    unthemed=$(echo $wrapper | sed 's/themed-//g')
    name=$(basename $unthemed)

    echo "  $name"

    rg-in-place "selector: ?'ds-" -r "selector: 'ds-unthemed-" $unthemed
    rg-in-place "selector: ?'ds-themed-" -r "selector: 'ds-" $wrapper

    for themed in $(find src/themes -name $name) ; do
        if [[ $themed =~ src/themes/([^/]+)/(.*) ]]; then
            theme=$match[1]
        fi

        rg-in-place "selector: ?'ds-" -r "selector: 'ds-$theme-" $themed
    done
done

echo "Updating usages in HTML"
for usage in $(rg -g '*.component.html' -l "<ds-themed-?") ; do
    echo "  $usage"
    rg-in-place '<ds-themed-' -r '<ds-' $usage
    rg-in-place '</ds-themed-' -r '</ds-' $usage
done

ybnd avatar Mar 01 '24 12:03 ybnd

@ybnd and @artlowel : Per today's discussion on this ticket in our DSpace DevMtg, a few notes:

  • There seems to be interest in this idea overall, as it seems to make theming easier.
  • Concerns expressed though about what the upgrade process might look like for institutions that are using custom themes:
    • Will all institutions need to change their themed component selectors to the new model of ds-[themename]-[component]? Or can their selectors remain as-is?
    • If all institutions need to change the selectors (or ideally should), then we may need to consider updating the above script to be a yarn based tool...that way other institutions could use it to perform these updates in a more automated fashion.
  • Some were finding it hard to visualize the new theming model... it might be good to add a specific example of how to make an existing component themable using the old way vs the new way. (The current details in the description are not specific enough, and that's where an example could be beneficial.)

Overall, I think this sounds promising. It's unclear to me though whether we will have time/interest to get this into 8.0 quickly. I asked others to comment here though with questions/thoughts to help us come to a quick consensus on direction.

tdonohue avatar Mar 07 '24 16:03 tdonohue

Here's a practical example: let's say we want to make the ChipsComponent themeable and add it to the custom theme

Current process

  1. Create a themed component wrapper
    • File: src/app/shared/form/chips/themed-chips.component.ts
    • Class: ThemedChipsComponent extends ThemedComponent<ChipsComponent>
    • Selector: ds-themed-chips
    • Declared in the same module where ChipsComponent is declared
  2. Change all instances of ds-chips in *.html to ds-themed-chips
    • If we make the component themeable on a fork, we may have to repeat this step if we merge in upstream changes later
  3. Create a themed version of the component in the custom theme
    • File: src/themes/custom/app/shared/form/chips/chips.component.ts
    • Import unthemed src/app/shared/form/chips/chips.component.ts as BaseComponent
    • Class: ChipsComponent extends BaseComponent
    • Selector: ds-chips
    • Declared in LazyThemeModule

Proposed new process

(changes are highlighted in bold)

  1. Create a themed component wrapper
    • File: src/app/shared/form/chips/themed-chips.component.ts
    • Class: ThemedChipsComponent extends ThemedComponent<ChipsComponent>
    • Selector: ds-chips
    • Declared in the same module where ChipsComponent is declared
  2. Change the selector of ChipsComponent to ds-base-chips
  3. Create a themed version of the component in the custom theme
    • File: src/themes/custom/app/shared/form/chips/chips.component.ts
    • Import unthemed src/app/shared/form/chips/chips.component.ts as BaseComponent
    • Class: ChipsComponent extends BaseComponent
    • Selector: ds-themed-chips
    • Declared in LazyThemeModule

ybnd avatar Mar 08 '24 12:03 ybnd

If all institutions need to change the selectors (or ideally should), then we may need to consider updating the above script to be a yarn based tool...that way other institutions could use it to perform these updates in a more automated fashion.

A simple search/replace approach would not be reliable in this case IMO. It's a good start, but makes a lot of assumptions and may require a lot of manual adjustments after -- hard to do when hundreds of files are affected.

Developing a custom linting/fixing setup seems like the best way forward. Moreover, once we have one we can leverage it for a number of other situations any time.

ybnd avatar Mar 15 '24 13:03 ybnd