Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

🙏 SPFx v1.13 beta20 web part template feedback - please don't bloat the default class #PleaseChange #7308

Closed
1 of 9 tasks
andrewconnell opened this issue Sep 9, 2021 · 6 comments
Labels
area:spfx Category: SharePoint Framework (not extensions related) Needs: Triage 🔍 Awaiting categorization and initial review.

Comments

@andrewconnell
Copy link
Collaborator

andrewconnell commented Sep 9, 2021

Target SharePoint environment

SharePoint Online

What SharePoint development model, framework, SDK or API is this about?

💥 SharePoint Framework

Developer environment

macOS

What browser(s) / client(s) have you tested

  • 💥 Internet Explorer
  • 💥 Microsoft Edge
  • 💥 Google Chrome
  • 💥 FireFox
  • 💥 Safari
  • mobile (iOS/iPadOS)
  • mobile (Android)
  • not applicable
  • other (enter in the "Additional environment details" area below)

Additional environment details

  • browser version (n/a)
  • SPFx 1.13.0 beta 20
  • Node.js 14.17.3

Describe the bug / error

TLDR

The new web part template introduced within the most recent beta (#20) for the upcoming 1.13.0 SPFx release is loaded with additional code that's not necessary. Please move this to a new ThemedBaseClientSideWebPart class & inherit from it.

More

The new base web part class is now loaded with theming bloat in the simplest "hello world" web part using no web framework. Specifically:

  • 5 new private members to support MS Teams themes
  • a 25-line onInit() method to support MS Teams themes
  • 6 new private methods to support MS Teams themes

IMHO, this is bad for multiple reasons:

  1. if you aren't building for MS Teams, the majority of this code is moot
  2. the first-run experience for a new developer is worse than before because they now have 220 LOC they have to figure out "where do I put my stuff? what can I remove? can I modify any of this?"
  3. if there's one typo in any of this boilerplate code, SPFx v1.13.1's release notes will be filled with "find this line & change it to the following..."
  4. if you're building a solution that will never be used within MS Teams, most of this code is irrelevant
  5. if you're building a solution that WILL be used in MS Teams, there's very little reason to change any of this code

Steps to reproduce

  1. install latest SPFx generator beta: npm install @microsoft/generator-sharepoint@next -g
  2. create a new project, accept all default values when prompted by the yeoman generator
  3. open the project in a text editor such as VS Code
  4. look at the base web part class... all 220 lines of it :(

Expected behavior

Default projects should not be filled with boilerplate code that's not intended to be modified. That code should be in base classes.

Proposed solution

Why not push ALL this stuff in the three bullet points above into a new class and inherit from it? For instance:

  1. create a new base class and include it within the @microsoft/sp-webpart-base:

    export class ThemedBaseClientSideWebPart<TProperties> extends BaseClientSideWebPart<TProperties> {}
  2. 100% of the three bullet items from above into this new class (including the private members & onInit()), along with the import dependencies & remove all that from the new web part class template

  3. change the web part class template to create a web part that looks like this:

    export default class HelloWorldWebPart extends ThemedBaseClientSideWebPart<IHelloWorldWebPartProps> {}
  4. bask in your coding glory

@andrewconnell andrewconnell added the area:spfx Category: SharePoint Framework (not extensions related) label Sep 9, 2021
@ghost
Copy link

ghost commented Sep 9, 2021

Thank you for reporting this issue. We will be triaging your incoming issue as soon as possible.

@ghost ghost added the Needs: Triage 🔍 Awaiting categorization and initial review. label Sep 9, 2021
@andrewconnell
Copy link
Collaborator Author

For those who haven't installed the beta, this is what you get with a new default class using beta 20 when creating a new web part and accept all defaults (ie: don't pick React)

import { Version } from '@microsoft/sp-core-library';
import {
  IPropertyPaneConfiguration,
  PropertyPaneTextField
} from '@microsoft/sp-property-pane';
import { BaseClientSideWebPart } from '@microsoft/sp-webpart-base';
import {
  ThemeProvider,
  ThemeChangedEventArgs,
  IReadonlyTheme,
  ITheme
} from '@microsoft/sp-component-base';
import { escape } from '@microsoft/sp-lodash-subset';

import styles from './HelloWorldWebPart.module.scss';
import * as strings from 'HelloWorldWebPartStrings';

export interface IHelloWorldWebPartProps {
  description: string;
}

export default class HelloWorldWebPart extends BaseClientSideWebPart<IHelloWorldWebPartProps> {

  private _themeProvider: ThemeProvider;
  private _themeVariant: IReadonlyTheme | undefined;
  private _isDarkTheme: boolean = false;
  private _environmentMessage: string = '';
  private _hasTeamsContext: boolean = false;

  protected onInit(): Promise<void> {
    this._hasTeamsContext = !!this.context.sdks.microsoftTeams;

    if (this._hasTeamsContext) {
      // handling MS Teams theme
      const teamsTheme = this.context.sdks.microsoftTeams.context.theme;
      this._updateTheme(teamsTheme);

      this.context.sdks.microsoftTeams.teamsJs.registerOnThemeChangeHandler(this._handleTeamsThemeChangedEvent);
    }
    else {
      // Consume the new ThemeProvider service
      this._themeProvider = this.context.serviceScope.consume(ThemeProvider.serviceKey);

      // If it exists, get the theme variant
      this._themeVariant = this._themeProvider.tryGetTheme();
      this._updateTheme(this._themeVariant);

      // Register a handler to be notified if the theme variant changes
      this._themeProvider.themeChangedEvent.add(this, this._handleSPThemeChangedEvent);
    }

    this._environmentMessage = this._getEnvironmentMessage();

    return super.onInit();
  }

  public render(): void {
    this.domElement.innerHTML = `
    <div class="${ styles.helloWorld } ${ this._hasTeamsContext ? styles.teams : '' }">
      <div class="${ styles.container }">
        <div class="${ styles.row }">
          <div class="${ styles.column } ${ styles.center }">
            <img src="${this._isDarkTheme ? require('./assets/welcome-dark.png') : require('./assets/welcome-light.png')}" class="${styles.welcomeImage}" />
          </div>
        </div>
        <div class="${ styles.row }">
          <div class="${ styles.column } ${ styles.center }">
            <span class="${ styles.title }">Well done, ${this.context.pageContext.user.displayName}!</span>
            <p class="${ styles.subTitle }">${this._environmentMessage}</p>
            <p class="${ styles.subTitle }">Web part property value: <span class="${ styles.description }"> ${escape(this.properties.description)}</span></p>
          </div>
        </div>
        <div class="${ styles.row }">
          <div class="${ styles.column }">
            <span class="${ styles.header }">Welcome to SharePoint Framework!</span>
            <p class="${ styles.content }">
            The SharePoint Framework (SPFx) is a extensibility model for Microsoft Viva, Microsoft Teams and SharePoint. It's the easiest way to extend Microsoft 365 with automatic Single Sign On, automatic hosting and industry standard tooling.
            </p>
            <span class="${ styles.subheader }">Learn more about SPFx development:</span>
            <p class="${ styles.content }">
              <ul>
                <li><a class="${styles.link}" href="https://aka.ms/spfx" target="_blank">Overview of the SharePoint Framework</a></li>
                <li><a class="${styles.link}" href="https://docs.microsoft.com/en-us/sharepoint/dev/spfx/web-parts/basics/notes-on-solution-packaging" target="_blank">SharePoint solution packaging</a></li>
                <li><a class="${styles.link}" href="https://docs.microsoft.com/en-us/sharepoint/dev/spfx/build-for-teams-overview" target="_blank">Build for Microsoft Teams using SharePoint Framework</a></li>
                <li><a class="${styles.link}" href="https://docs.microsoft.com/en-us/sharepoint/dev/spfx/web-parts/get-started/use-fabric-react-components" target="_blank">Use Fluent UI components in your SharePoint client-side web part</a></li>
                <li><a class="${styles.link}" href="https://docs.microsoft.com/en-us/sharepoint/dev/spfx/web-parts/get-started/using-microsoft-graph-apis" target="_blank">Use Microsoft Graph in your solution</a></li>
                <li><a class="${styles.link}" href="https://docs.microsoft.com/en-us/javascript/api/overview/sharepoint?view=sp-typescript-latest" target="_blank">SharePoint Framework API reference</a></li>
                <li><a class="${styles.link}" href="https://aka.ms/m365pnp" target="_blank">Microsoft 365 Community Assets - Calls, samples and documentation</a></li>
                <li><a class="${styles.link}" href="https://aka.ms/m365/devprogram" target="_blank">Register to Microsoft 365 Developer program for a free developer tenant</a></li>
              </ul>
            </p>
          </div>
        </div>
      </div>
    </div>`;
  }

  /**
   * Update the current theme variant reference and re-render.
   *
   * @param args The new theme
   */
   private _handleSPThemeChangedEvent = (args: ThemeChangedEventArgs): void => {
    this._themeVariant = args.theme;
    this._updateTheme(this._themeVariant);
    this.render();
  }

  /**
   * Handle Teams theme change callback.
   *
   * @param args The new theme
   */
   private _handleTeamsThemeChangedEvent = (theme: string): void => {
    this._updateTheme(theme);
    this.render();
  }

  /**
   * Updates fields based on the new theme
   * @param currentTheme updated theme
   */
  private _updateTheme = (currentTheme: IReadonlyTheme | string) => {
    this._setIsDarkTheme(currentTheme);
    this._setCSSVariables();
  }

  /**
   * Updates the _isDarkTheme based on current SharePoint or Teams theme
   */
  private _setIsDarkTheme = (currentTheme: IReadonlyTheme | string) => {
    if (typeof currentTheme === 'string') { // Teams theme
      this._isDarkTheme = currentTheme !== 'default'; // contrast theme is interpreted as dark
    }
    else { // SharePoint theme
      const theme = currentTheme as ITheme;
      this._isDarkTheme = !!theme && !!theme.isInverted;
    }
  }

  private _setCSSVariables = () => {
    let primaryText = '#323130'; // default
    let linkText = '#03787c';
    if (this._themeVariant) {
      const {
        semanticColors
      } = this._themeVariant;
      primaryText = semanticColors.bodyText;
      linkText = semanticColors.link;
    }
    else if (this._hasTeamsContext) { // fallback for Teams
      primaryText = this._isDarkTheme ? '#FFFFFF' : '#242424';
      linkText = this._isDarkTheme ? '#FFFFFF' : '#494B83';
    }
    else { // fallback for single app page
      primaryText = this._isDarkTheme ? '#3a96dd' : '#323130';
      linkText = this._isDarkTheme ? '#3a96dd' : '#03787c';
    }

    this.domElement.style.setProperty('--primaryText', primaryText);
    this.domElement.style.setProperty('--linkText', linkText);
  }

  private _getEnvironmentMessage = (): string => {
    // checking for local environment
    let isLocal: boolean = false;
    const {
      loaderConfig
    } = this.manifest;

    if (loaderConfig && loaderConfig.internalModuleBaseUrls && loaderConfig.internalModuleBaseUrls.length) {
      isLocal = /^http(s)?\:\/\/localhost/gmi.test(loaderConfig.internalModuleBaseUrls[0]);
    }

    if (this._hasTeamsContext) { // running in Teams
      return isLocal ? strings.AppLocalEnvironmentTeams : strings.AppTeamsTabEnvironment;
    }

    return isLocal ? strings.AppLocalEnvironmentSharePoint : strings.AppSharePointEnvironment;
  }

  protected onDispose(): void {
    //
    // unregistering theme changed handlers
    //
    if (this._hasTeamsContext) {
      this.context.sdks.microsoftTeams.teamsJs.registerOnThemeChangeHandler(null);
    }
    else {
      this._themeProvider.themeChangedEvent.remove(this, this._handleSPThemeChangedEvent);
    }
  }

  protected get dataVersion(): Version {
    return Version.parse('1.0');
  }

  protected getPropertyPaneConfiguration(): IPropertyPaneConfiguration {
    return {
      pages: [
        {
          header: {
            description: strings.PropertyPaneDescription
          },
          groups: [
            {
              groupName: strings.BasicGroupName,
              groupFields: [
                PropertyPaneTextField('description', {
                  label: strings.DescriptionFieldLabel
                })
              ]
            }
          ]
        }
      ]
    };
  }
}

@coreyroth
Copy link

I just noticed the change as well. While I welcome proper guidance on how to properly support Microsoft Teams Themes including things like handling a theme change, this certainly is intimidating for a new user just getting started.

@juliemturner
Copy link
Contributor

I agree that I think it's excellent to provide guidance, I'm not in favor of this code being included in the base web part in this manner by default. I do not do theming support in this manner and don't want the added overhead of having to remove all this code every time, I start a new project.

@patmill
Copy link
Contributor

patmill commented Sep 9, 2021

I'm going to figure out how to move this to the discussion feature in github.

@VesaJuvonen
Copy link
Contributor

Few comments and let's move this to the discussion side for clarity as it's more discussion, rather than an issue. This is in the beta release and it's intended for having these kind of discussions.

  • Objective is to move stuff to base class to clean up things and to make it easier to use those options as needed
  • We want to have clean and fully functioning web part AND teams experience available for show casing the art of possible
  • There's two main parts - make the default template to support Teams and to support section background in SharePoint
  • In similar ways as with the previous web part versions - you can delete the unnecessary lines - you only have few more lines to delete for getting the web part cleaned

Why did we do this?

  • We get feedback constantly around non-optimal experience for web parts in Teams - so now even the default project is optimized for that
  • Old template did not understand section background settings and was not aware of Teams context
  • Removing few more lines vs the older template was not considered as a huge task for developers who want to clean up things

I'm moving this to discussions side to keep the issue list more specifically for issues. Similar clean up is happening for older issues.

@SharePoint SharePoint locked and limited conversation to collaborators Sep 9, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
area:spfx Category: SharePoint Framework (not extensions related) Needs: Triage 🔍 Awaiting categorization and initial review.
Projects
None yet
Development

No branches or pull requests

5 participants