SPFx v1.13 beta20 web part template feedback - HTML/CSS perspective - 🙏 Please change it too

[StfBauer]

I love the new design of this web part, It is simple and provides value to every new project.

The HTML and CSS sadly this web part has not been properly implemented and is in many ways misleading. In some cases, it does not even container valid markup.

From this perspective, the old web part was pretty solid and gave a good example of an implementation.

IMO the content of the web part should be brought to 2021 and not show rendered markup CSS definitions that are supported already by IE9. IMO this content should not be served to developers in the current state.

Line numbers reference to No-Framework Web Part.

HTML:

Element as the container class - Line: 59

<div class="${ styles.helloWorld } ${ this._hasTeamsContext ? styles.teams : '' }">

Better would be to use a <section> instead of a div, for every web part. The reason can be found in the definition.

The

element represents a generic section of a document or application. A section, in this context, is a thematic grouping of content. Each section should be identified, typically by including a heading (h1-h6 element) as a child of the

element. - https://www.w3.org/TR/html/sections.html#the-section-element

HTML: .container inside not needed - Line: 60

<div class="${ styles.container }">

This container is not needed. It does not serve any real purpose only than add the grid to the web part. The grid could be directly assigned on:

.helloWorld{
    @include ms-Grid;
    color: var(--primaryText, #323130); // note: CSS variables support is limited to modern browsers only
}

HTML / CSS - Grid system

The web part was built using the Office UI Fabric Core - Grid system. The code is completely outdated even for IE11. This could be removed and replaced with, for example, a flex-box.

There is also another architectural issue with the grid-system in a web part. The main purpose is for pages not for components on a page, the legacy grid in the "Hello World Web Part" has never worked properly. ->

The grid in the web part does not help to align the content to the page grid overall. Is Office UI Fabric Core still supported?

Row / Columns - Long live IE10

Because the web part uses the Office UI Fabric Core Grid System, the web part uses also row / container container.

<div class="${ styles.row }">
   <div class="${ styles.column } ${ styles.center }">

Grid like these was already implemented already 14 years ago. Ripping out the failing CSS grid would also make this container obvious.

HTML Hero Image - Line: 62

<div class="${ styles.column } ${ styles.center }">
<img src="${this._isDarkTheme ? require('./assets/welcome-dark.png') : require('./assets/welcome-light.png')}" class="${styles.welcomeImage}" />
</div>

'style.center' works technically but CSS classes should be descriptive like that. TailWind uses AtomicCSS which has class names like this but that's a different story.

Better name it, what it is. For example:

• .welcome > welcomeImage
• mediaBlock
• headerImage
• hero

In addition to moving all required styles in this class.

Accessibility (A11y) - Hero Image

The image misses an alt="" attribute or role="presentation" it is only "decorative".

See: https://www.w3.org/WAI/tutorials/images/decorative/

SPFx: Image was not changed

No Framework generator: According to the definition the logo should get changed, when the theme changes. The web part hasn't rendered the section new and replace HTML with another image.

I also don't like that every developer needs to think about deleting the images from the solution not to bundle them in with every new project. One image with a CSS background for example a white circle will solve this issue for light and dark themes in Microsoft Teams, as well as Dark section background.

Optimal SVG image, that will be deleted, when the web parts get altered. Style changes can directly reflect in the SVG too.

Title, Subtitle, Subtitle? - Line: 68 - 70

First <span> element, that is inline elements and not block elements it wrong here.

<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>

The content has classes defined with a title, which only renders like a block element because it is followed by a paragraph.

A paragraph used as 'style.subTitle'? When those are "real" titles and subtitles, then this should be represented in the DOM too. H1 - H6 over a lot of options here, when they are in a <section> those are safe to be used there.

Especially subTitle is a useless definition:

  .subTitle {
    @include ms-fontSize-14;
  }

Font size 14 is already defined on a page level, so it does not need to be extra assigned here. On the other hand, if SharePoint changes at some point to font-size-16 for example this will still render with 14px.

Textual issue on description / web part properties - Line: 70

Web part property value: <span class="${ styles.description }"> ${escape(this.properties.description)}</span>

What web part property is shown here? Should the person who renders the web part the first time know which property. Can there be only one web part property?

HTML - for description

To render a bold text HTML has luckily the <strong> tag. So the style definition is completely useless.

  .description {
    @include ms-fontWeight-bold;
    @include ms-fontSize-14;
  }

Font size is one of those properties that get also applied to the child elements. So the already defined ms-fontsize-14 gets applied to the paragraph. With using the <strong> that the ms-fontWeight-bold get applied without CSS involvement.

HTML - <span> Header - Line: 75

We already saw the title, subtitle, subtitle and now header as a span element. The same issue as before - span gets rendered as block just because of the following paragraph.

<span class="${ styles.header }">Welcome to SharePoint Framework!</span>

For consistency, a proper style H1-H1 headline style would serve the web part better. Especially the also have the same style definitions.

.title {
    @include ms-fontSize-18;
    @include ms-fontWeight-semibold;
}

.header {
    @include ms-fontSize-18;
    @include ms-fontWeight-semibold;
}

HTML P.content? - Line: 76

Was the rest from before not content?

<p class="${ styles.content }">
The SharePoint Framework (SPFx) is an 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>

CSS P.cotent?

We already had the subTitle style from before, which is basically the same as we have now on p.content.

.subTitle {
	@include ms-fontSize-14;
}
.content {
	@include ms-fontSize-14;
}

I don't understand why the CSS is bloating like this. It does not make sense and is really bad practice. Especially the same elements have been named completely different.

HTML / CSS Subheader - But Why? Line: 79

The subhead style we have already seen before.

<span class="${ styles.subheader }">Learn more about SPFx development:</span>

What makes this so special than a .description? It has the same definition and an h2, h3 would be anyway more appropriate in case of the structure of the content as well as accessibility.

Well, I found a difference between both definitions.

.subheader {
	@include ms-fontSize-14;
	@include ms-fontWeight-semibold;
}
.description {
	@include ms-fontWeight-bold;
	@include ms-fontSize-14;
}

One start with 'ms-fontSize-14' while the other starts with 'ms-fontWeight-bold'.

HTML - Major flaw of incorrect markup - Line: 80 - 91

Last but not least there is one big issue with the validity of the markup at the end that do not happen by accident IMO. A paragraph can never ever contain an ordered list or unordered list.

<p class="${ styles.content }">
  <ul>
    <li>
  </ul>
</p>

Not even browser like this and render the incorrect code in the following way:

<p class="content_bc69a16d">
</p> 
<ul>
    ...
</ul>
<p></p>

Paragraph - Consistancy

For consistency reasons it would be better than the paragraph will follow the Rich Text Editor definitions, would bring in more regular CSS and not only SASS @includes.

Sloppy link definition

It is ok to define a style for links but then the should be properly defined. It would also give developers a better idea of how to implement style, for example for links.

.link {
	color: var(--linkText, #03787c); // note: CSS variables support is limited to modern browsers only
}

The following state on the like are missing:

• hover
• active
• visited

CSS Variable use

.link {
	color: var(--linkText, #03787c); // note: CSS variables support is limited to modern browsers only
}

Per definition in the CSS Specification there is no support for fallback values. In worst case no color get applied, because of an invalid definition. This is pure browser specific code.

The official name is also CSS custom properties (Spec Name: CSS Custom Properties for Cascading Variables Module Level 1) and not CSS Variables.

Modern browser is also pretty misleading for an official sample web part. Better would be.

To see browser support for CSS custom properties - https://caniuse.com/?search=css%20custom%20properties

Also a real fallback for browser that do not support CSS variables would be nice too. Even though when a browser do not support var the code gets ignored and does not fallback to '#03787c'.

So a proper definition would look like this:

.link {
	color: #03787c;
	color: var(--linkText); // note: CSS variables support is limited to modern browsers only
}

CSS Nesting not required

Due to the use fo CSS modules there is no reason whatsoever to net all style sheet classes inside the .helloWorld` definition other that bloat the resulting CSS.

Perfect example for nesting would be a proper link definition.

UL/Li - Do not need styling to follow RTE definition on SharePoint

UL/LI is completely un-styled. Would't it be better to follow the rich text editor styles?

Other considerations

• Cherry picking and fixed values in this code should be changed.

  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);
  }

Especially:

    let primaryText = '#323130'; // default
    let linkText = '#03787c';

To what is this default? Why fixed colour that only works in one theme. -> things like this already in SPFx v1.13 beta20 web part template feedback - please don't bloat the default class #PleaseChange

// fallback for single app page

The official name is "Single Web Part App Page" an I really would love that this:

this._themeProvider = this.context.serviceScope.consume(ThemeProvider.serviceKey);

Would work there too - Fallback to window.__themeState__.theme will even work limited in Microsoft Teams.

High Contrast in Teams\

High contrast for accessibility should be supported too.