Skip to content

Conversation

@Westbrook
Copy link
Contributor

Description

  • removes the fragment minimum before a "theme" is applied to sp-theme
  • moves to an async rendering pattern to prevent multiple renders as various fragments are loaded
  • adds support for an app fragment so that consuming application can hang local extensions of the Spectrum CSS Custom Properties API on the sp-theme element

Questions:

  • This approach does not support for more than one app fragment, is that OK?
  • This approach does not support removing the app fragment, is that OK?
  • @stanvladut pointed out how his team was previously overload fragments to achieve a similar goal, should we prevent that in some way?
  • Is it OK to allow "partial" themes or should we ensure one of each fragment type via a more rigorous check?
  • Are there edge/use cases that I've not thought of here?

Related Issue

fixes #1496

Motivation and Context

Easier customization of applications consuming Spectrum Web Component and associated styling APIs

How Has This Been Tested?

  • updated existing tests
  • added new tests
  • found acceptable VRT failures and adopted them (when we agreed that they were acceptable)

Types of changes

  • Refactor
  • New feature (non-breaking change which adds functionality)
  • Possibly Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Contributor

@stanvladut stanvladut left a comment

Choose a reason for hiding this comment

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

Exepting the minor comment I posted, the changes look good, great job. Thanks for working on this and proposing a solution so fast!

My opinion on some of the questions posted:

  1. Generally speaking, I think it's enough (at least for my team) to only have one app fragment. We can compute our app styles locally, depending on color, scale or other factors and register it when ready.
  2. I cannot think of a scenario where we would want that, as long as we can change the styles for the app fragment. Keeping in mind that the local app styles might also depend on color or scale, we might want to recompute the app styles and update them, but not remove.

if (currentStyles) {
acc.push(currentStyles.styles);
const addStyles = (
name: FragmentType,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be of type FragmentName, as it can have values other than 'color' | 'scale' | 'core' | 'app'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Even my own typescript alludes me more than I'd like! 🙈

@Westbrook Westbrook force-pushed the westbrook/custom-theming branch from e7accb8 to 4fed6b1 Compare June 9, 2021 18:56
@Westbrook Westbrook force-pushed the westbrook/testing branch 3 times, most recently from 76b4793 to 5bd662d Compare June 10, 2021 18:43
Base automatically changed from westbrook/testing to main June 10, 2021 18:51
@Westbrook Westbrook force-pushed the westbrook/custom-theming branch 2 times, most recently from ae4f12f to 29fbd44 Compare June 14, 2021 15:25
najikahalsema
najikahalsema previously approved these changes Jun 14, 2021
stanvladut
stanvladut previously approved these changes Jun 15, 2021
@Westbrook Westbrook dismissed stale reviews from stanvladut and najikahalsema via d0cbdc1 June 15, 2021 12:16
@Westbrook Westbrook force-pushed the westbrook/custom-theming branch 3 times, most recently from 8a8249c to 004527d Compare June 16, 2021 15:14
stanvladut
stanvladut previously approved these changes Jun 23, 2021
Copy link
Contributor

@stanvladut stanvladut left a comment

Choose a reason for hiding this comment

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

Great work! 💯

@Westbrook Westbrook force-pushed the westbrook/custom-theming branch 3 times, most recently from 9684996 to d273341 Compare June 24, 2021 12:08
@Westbrook Westbrook changed the title [RFC] feat: remove theme fragment minimum and allow "app" fragments feat: remove theme fragment minimum and allow "app" fragments Jun 24, 2021
@Westbrook Westbrook force-pushed the westbrook/custom-theming branch 2 times, most recently from 7aaaafd to 67e1441 Compare June 24, 2021 16:28
stanvladut
stanvladut previously approved these changes Jun 24, 2021
Copy link
Contributor

@stanvladut stanvladut left a comment

Choose a reason for hiding this comment

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

Great work!

@Westbrook Westbrook force-pushed the westbrook/custom-theming branch from 5d1f4fb to d6ef939 Compare June 24, 2021 19:33
@Westbrook Westbrook merged commit 982928d into main Jun 24, 2021
@Westbrook Westbrook deleted the westbrook/custom-theming branch June 24, 2021 19:41
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.

[Theme] Allow Theme component to provide custom styles

4 participants