🙏 SPFx v1.13 beta20 web part template feedback #7310
Replies: 15 comments 17 replies
-
|
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
})
]
}
]
}
]
};
}
} |
Beta Was this translation helpful? Give feedback.
-
|
Ouch!!! In my view you should not have to delete code you don't need. It's much better to add the code you need instead. Otherwise, it can get very confusing for developers relatively new to SPFx (like me). The questions that I would have looking at that code would be:
A based class that you can derive from if you need that functionality would be my choice (and the corresponding update on the documentation). |
Beta Was this translation helpful? Give feedback.
-
|
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. |
Beta Was this translation helpful? Give feedback.
-
|
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. |
Beta Was this translation helpful? Give feedback.
-
|
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.
Why did we do this?
I'm moving this to discussions side to keep the issue list more specifically for issues. Similar clean up is happening for older issues. |
Beta Was this translation helpful? Give feedback.
-
|
One discussed option here was to introduce new switch for the Yeoman generator and then keep the clean one as the default option with only following Render() - would help on reducing the number of lines to get deleted when new solution is created or web part added. public render(): void {
this.domElement.innerHTML = escape(this.properties.description);
}This is the core value of public betas - let's keep the discussion ongoing. We want to hear widely from our partners and customers on their input on this for sure. |
Beta Was this translation helpful? Give feedback.
-
I'll go back to my initial reply: |
Beta Was this translation helpful? Give feedback.
-
|
Personally, I have nothing against new switches or new questions in the generation flow, as long as they are clear enough in the description/question and if there is clear documentation explaining their purpose. Don’t get me wrong, but I think that we should craft the SPFx development experience thinking about everyone’s needs and not thinking about "black belt" ninjas' needs. I mean: most of the people writing here are working on SPFx on a daily basis, and as such we are all used to removing/replacing/customizing/tailoring code to achieve our goals in the best way. All good, indeed. However, we need to think about people who would like to learn from the basics and to easily create SPFx solutions without having to grab code snippets here and there on the web to build a fully working solution. Having a generator that will generate almost “nothing” would be not that useful IMHO. Options and switches can help both experienced SPFx developers and newby to choose what to include and what to not include in the generated code, based on their real needs and on a project basis. |
Beta Was this translation helpful? Give feedback.
-
|
FWIW, I don't build many SPFx components & work primarily with people new to the platform (that's sort of my primary business, teaching people new to SPFx). My feedback is thinking primarily about their experience and from their perspective. Again, back to my initial proposal above, the point of feedback I raised was limited to moving all the theming stuff to a base class that anyone can opt into, or easily opt-out of. Not to propose going "emptier" than what the existing 1.12.1 web part template includes. If someone wants to create a new discussion point that focuses on the topic of making a richer web part template or empty template, go for it... but it's not constructive to fork the thread from the initial point I raised. |
Beta Was this translation helpful? Give feedback.
-
|
Having a base class is something that I like, as long as we will be able to override its default behavior when needed. So, I'm hearing you and I'm not against your initial feedback. At the same time, having additional switches or clear questions in the generation process, to define what you need to get generated and to determine the initial base class to inherit from, is an additional option that I'd like to have. I work a lot on real projects with customers and every now and then I do provide training to newby people. In both scenarios, nobody ever complained about the number of arguments/switches of the generator. Actually, a lot of people like the PnP generator, which asks you even more questions but gives you more fine-grained control on the output. |
Beta Was this translation helpful? Give feedback.
-
|
Sorry... @PaoloPia, I wasn't saying the opinion wasn't valid. Apologies if that's how it was conveyed. My point was more about making sure the discussion about creating more questions in the generator vs. creating the base class wasn't mixed into the same debate because that, at least to me, creates it an either/or. From my POV, they're two distinct questions. Having more questions in the generator is definitely a valid debate... I do have an opinion on it, but that's separate from what I was trying to convey in my initial post here. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks @andrewconnell and others on the feedback and this is really valuable and important discussion. Let's be highly tactical here with few questions for everyone on this thread.
We are of course evaluating the feedback and input from numerous perspective and trying to balance the optimal outcome with the feedback from new developers, customers, partners, internal partners, strategic direction and more. Thank you for your input advance. |
Beta Was this translation helpful? Give feedback.
-
Mostly what my initial suggestion that kicked off this thread stated (I was trying o be part of the solution rather than part of the problem from the start): all the new theming code moved to a base class, but mark the methods as
These can go into a NEW base class (as I proposed above), but not replace the existing one, as some people will be using their own theme implementation from before. That way, people can opt-in or opt-out by replacing the default web part's base class the new web part extends from depending on their use. Since it's clear from this change in beta 20 that Microsoft feels the theming should be baked into all web parts, makes sense to make this more of an opt-out experience, so new web parts would inherit from my proposed
Furthermore, I'd remove the stuff that 100% of the developers creating a new project will include in their production projects 0% of the time, including:
WHY? All three of these will always get deleted by a developer. The only text that needs to be in the default rendering is something that shows the different contexts when the web part is rendered to show how that works... just something minimal... I could make the same argument as above, but maybe someone won't delete it, rather they'll modify it. This differs from the stuff in my bullet list above. |
Beta Was this translation helpful? Give feedback.
-
|
I can say why this kind of web part could be helpful for me, even as an experienced SPFx dev. If the main logic of theme handling is encapsulated in base class with ability to override something like @juliemturner - you mentioned that you handle themes in completely other way. Could you please share that with me? |
Beta Was this translation helpful? Give feedback.
-
|
And yes - I don't feel bad of deleting unnecessary lines of code. I honestly don't see that as a problem. |
Beta Was this translation helpful? Give feedback.
-
|
@AJIXuMuK - By this, in general, I mean that I put all theme handling in a separate class (which will in the future be built into the design framework I use) and not in the root web part. That way my onInit just has 2 lines. But in specificity the implementation I see there seems incomplete compared to what I use from @StfBauer as it doesn't include fonts, palette, effects... etc... but maybe if you're not using HTWOO then you don't need them and also, I did not do an exhaustive comparison. Regardless, this is the way I do it and will continue to do it, so for me I would need to remove the code (and the extra assets, etc), and if created revert to a base class. All of which is fine but it's a lot. I would be in favor of switches or questions as like @PaoloPia they don't bother me really and there are cases where I do want some of those other things for testing and demos etc. |
Beta Was this translation helpful? Give feedback.
-
|
There is one piece missing which is essential for all of this kind of theming stuff - Don't forget about accessibility and the High Contrast theme is nowhere mentioned. Based on the Windows Settings SharePoint can handle high contrast pretty well, but this might work only on Windows. At least I haven't checked any other OS yet. When teams already offer the High Contrast Setting, then SharePoint should also have a switch or property that gets reported back and rendered then the web part in High Contrast. Maybe with a dedicated theme. |
Beta Was this translation helpful? Give feedback.
-
|
It’s a dilemma, because I find that new developers tend to “forget” to make their web parts responsive to theme events, and have been brought in a couple of times to make their webparts “theme aware”. I think the inheritance model @andrewconnell suggests is an excellent one, and also make it an option for yeoman, like where you choose ReactJS or JavaScript, have another step “scaffold theme code” or not. Currently, 1.12.1, I have a template project with all the theme code that I copy paste to every new project I do, so an option would be great. I like the new template provided as I have experience with building webparts, but it would be difficult for a beginner to webpart development. Hiding it all away in a base class seems the right approach as it is pretty fundamental to a working production webpart |
Beta Was this translation helpful? Give feedback.
-
|
I'm joingning @andrewconnell about putting all that Teams stuff in a But if the |
Beta Was this translation helpful? Give feedback.
-
|
I agree that there should be a simpler way to support Teams and SPO Themes with default setup. There have been a few different approaches the years in doing so and I can imagine that those that have invested the time to figure things out are happy to continue along with their current approaches. Or maybe they'll want to update their existing code to take advantage of things "on their own time" allowing them to remove their custom Theme/Teams support in favor of the OOB ones. I am in favor of supporting all these scenarios in a way that gives developers in these different scenarios the ability to do so in a way that gets folks falling down the "pit of success" quickly without lots of decision. An idea - what about like in ASP.NET projects in Startup.cs we configure things. Something like that applied to OnInit in web parts? this.SupportThemes() |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for all the feedback here. I had the exact same idea as @brianpmccullough . I personally don't like the "here's another class in the hierarchy to inherit from". We're going to revert the change for the 1.13 build (this isn't something that needs to be rushed) and include it in the first 1.14 beta, and iterate on it. |
Beta Was this translation helpful? Give feedback.
-
|
Fair point... just to clarify my proposal above, I was suggesting the default base class new web parts inherit from ( This way, Microsoft & many developers get what they want: a new web part that has theming baked in. But for those who have their own implementation of handling themes, or who don't want it for whatever reason, they can easily opt-out by changing the base class of new web parts to what we use in the pre-1.13.0.beta20 web parts: Sure, you could do the same thing today with what's in 1.13.0.beta20, by deleting the private members, private methods, |
Beta Was this translation helpful? Give feedback.
-
|
@patmill This would mean that all theme tokens will be automatically added to every new web part. Similar to what I did here. // 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();
console.debug('Theme variant ::: ', this._themeVariant);
// If there is a theme variant
if (this._themeVariant) {
// we set transfer semanticColors into CSS variables
this.setCSSVariables(this._themeVariant.semanticColors);
}There are also a couple of other style and theme related information, which are currently neglected by the new web part implementation.
This cherry-picking 🍒 of color information that happens at the current implementation, do not really have a positive impact on the performance but leads to a lot of avoidable clutter in the code. |
Beta Was this translation helpful? Give feedback.
-
|
I really love that the new web part "looks" cleaner but the code of the HTML is not there where it should be. Sadly it was not properly implemented and has a lot of room for improvement. This from my perspective is also essential to give new and existing developers a good impression. This threat here is more an SPFx development-driven discussion. I decided to create another discussion specific to HTML and CSS. TLDR: The old HTML/CSS was ok - now it is broken. The complete review is a bit longer so I moved it to its own threat - #7334 (SPFx v1.13 beta20 web part template feedback - HTML/CSS perspective - 🙏 Please change it too) |
Beta Was this translation helpful? Give feedback.
-
|
Just updating this thread - we have now updated templates available as part of the 1.14 preview release. Input is more than welcome - see release notes at https://docs.microsoft.com/en-us/sharepoint/dev/spfx/release-1.14 |
Beta Was this translation helpful? Give feedback.
-
|
New Feedback - #7568 |
Beta Was this translation helpful? Give feedback.
-
|
Thanks Stefan. I'm going to lock this one now. |
Beta Was this translation helpful? Give feedback.
-
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
Additional environment details
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
ThemedBaseClientSideWebPartclass & 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:
onInit()method to support MS Teams themesIMHO, this is bad for multiple reasons:
Steps to reproduce
npm install @microsoft/generator-sharepoint@next -gExpected 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:
create a new base class and include it within the @microsoft/sp-webpart-base:
100% of the three bullet items from above into this new class (including the private members &
onInit()), along with theimportdependencies & remove all that from the new web part class templatechange the web part class template to create a web part that looks like this:
bask in your coding glory
Beta Was this translation helpful? Give feedback.
All reactions