feat: add fast-components-msft as a new package #3096

chrisdholt · 2020-05-09T18:59:08Z

Description

This PR is ~~the initial work for~~ closes #3088. This PR creates a new package for fast-components-msft. The rewrite of existing styles for the FAST design system ~~is not included as part of this PR.~~ has been completed and a new package for fast-foundation was created to hold the templates, classes, and utilities needed to compose web components with FAST.

Issue type checklist

Chore: A change that does not impact distributed packages.
Bug fix: A change that fixes an issue, link to the issue above.
New feature: A change that adds functionality.

Is this a breaking change?

This change causes current functionality to break.

Process & policy checklist

I have added tests for my changes.
I have tested my changes.
I have updated the project documentation to reflect my changes.
I have read the CONTRIBUTING documentation and followed the standards for this project.

chrisdholt · 2020-05-11T16:31:04Z

packages/web-components/fast-components-msft/src/badge/index.ts

+export class MSFTBadge extends Badge {
+    @attr
+    public fill: BadgeFill = "lightweight";
+    private fillChanged(oldValue: BadgeFill, newValue: BadgeFill): void {
+        if (oldValue !== newValue) {
+            this.classList.add(newValue);
+            this.classList.remove(oldValue);
+        }
+    }
+
+    // Can we remove this and circular as attrs if we don't want to leverage them?
+    // Can I set this internally only in this implementation?
+    @attr({ mode: "fromView" })
+    public color: string = "value";
+}


@microsoft/fast-dna-collaborators my thoughts on the implementation of Badge here was to tighten up the implementation to better align with the MSFT badge requirements. Ideally I'd like to not provide the color value here and just have things work as expected. Since we cannot remove or delete attributes from the classes here, I'm trying to consider how to manage the mappings.

It seems like having defaults for both fill and color make sense; while not illustrated above we could also ensure the extensibility by also supporting a string:

export type BadgeFill = "accent" | "lightweight" | "neutral" | string;

I'm not sure what to do with the color attribute in those scenarios though if we elect to handle them in the stylesheet.

The fast-components api would remain the same?

I think this is part of the question...since we can't pull off existing attrs, extending from the Badge would expose all of those values - so yes, the API would stay the same. I think providing some default "named" configurations is helpful here, but wondering what our language/naming should align to. Recipes?

I think the naming seems right to me. It aligns with what we do elsewhere.

The one thing about our react badge is that the filled version was yellow and not aligned with the accent color. So might want @bheston to chime in here.

Yeah, I'd like some thoughts on the defaults - we can still keep things extensible. My concern with the yellow is that it won't respond to dark mode unless we leverage the provider which seems heavy-handed for a default value.

Bigger question in my mind is what to do with the color value (if anything)...do we provide it but not include a default value b/c the fill defaults will be recipe associated?

One option might be to move color so that it's defined on FASTBadge rather than Badge.

@EisenbergEffect there would be changes to the template as well I think - those values are set in a nested div as they are setting style attributes. I wonder if the deltas here are worth different API's or if we should keep consistent for the time being.

Ah, I see. Well, then it becomes a matter of what is worse, having some properties that don't apply vs. writing a whole new component.

chrisdholt · 2020-05-11T16:33:15Z

packages/web-components/fast-components-msft/src/divider/fixtures/divider.html

+    <fast-divider></fast-divider>
+
+    <h4>Presentation</h4>
+    <fast-divider role="presentation"></fast-divider>


@microsoft/fast-dna-collaborators We've talked about having two different recipes based on whether this needs to be accessible to a screen reader. In the event that the divider is only for presentation, the recipe would not need to meet contrast requirements. In the event that it is exposed to AT, the recipe would meet contrast requirements. I think we should consider adding this for the MSFT implementation here. @bheston - thoughts on the recipe to use for the contrast requirement implementation?

chrisdholt · 2020-05-11T17:10:45Z

packages/web-components/fast-components-msft/src/design-system-provider/index.ts

+import {
+    designSystemProvider,
+    FASTDesignSystemProvider,
+} from "@microsoft/fast-components";
+
+@designSystemProvider("msft-design-system-provider")
+export class MSFTDesignSystemProvider extends FASTDesignSystemProvider {}


@nicholasrice does this map to what you might expect here? I'd expect the default values to change here once we create the FAST system.

eljefe223 · 2020-05-11T17:31:04Z

Smoked to ensure everything is working properly

packages/web-components/fast-components-msft/README.md

EisenbergEffect

Main feedback to change the import format so that it adds the .js file extension for internal module imports.

EisenbergEffect · 2020-05-11T17:59:31Z

packages/web-components/fast-components-msft/src/anchor/index.ts

@@ -0,0 +1,14 @@
+import { customElement } from "@microsoft/fast-element";
+import { Anchor, AnchorTemplate as template } from "@microsoft/fast-components";
+import { AnchorStyles as styles } from "./anchor.styles";


In order to prep for the new build setup, let's start this new package with file extension in imports. So, change to this:

import { AnchorStyles as styles } from "./anchor.styles.js";

yeah, I wanted to do that work in a separate branch as part of the feature branch work. I can do in this package, but we'll need both packages to be touched prior to the feature branch merging in

EisenbergEffect · 2020-05-11T18:27:30Z

packages/web-components/fast-components-msft/src/badge/index.ts

+export class MSFTBadge extends Badge {
+    @attr
+    public fill: BadgeFill = "lightweight";
+    private fillChanged(oldValue: BadgeFill, newValue: BadgeFill): void {
+        if (oldValue !== newValue) {
+            this.classList.add(newValue);
+            this.classList.remove(oldValue);
+        }
+    }
+
+    // Can we remove this and circular as attrs if we don't want to leverage them?
+    // Can I set this internally only in this implementation?
+    @attr({ mode: "fromView" })
+    public color: string = "value";
+}


One option might be to move color so that it's defined on FASTBadge rather than Badge.

EisenbergEffect

One thing I've been wondering about...should we use a different element prefix for each design system or not?

We've got "fast-" and now "msft-" for the element names. This will make it hard for people to switch out design systems. They'll have to re-write all their html. Also, the scenario where somebody builds a single UI and then just dynamically loads different design systems based on platform (ios, vs. material vs. msft) isn't really possible because the element names are different. If the element names were the same, then the design language and implementation could simply be switched at load.

Also, with different element names, it feels a bit more like different components as apposed to the same components with a different design system, at least to me.

In any case, I wanted to bring this up as I'm not sure if we've thought through these scenarios.

chrisdholt · 2020-05-12T19:06:41Z

One thing I've been wondering about...should we use a different element prefix for each design system or not?

We've got "fast-" and now "msft-" for the element names. This will make it hard for people to switch out design systems. They'll have to re-write all their html. Also, the scenario where somebody builds a single UI and then just dynamically loads different design systems based on platform (ios, vs. material vs. msft) isn't really possible because the element names are different. If the element names were the same, then the design language and implementation could simply be switched at load.

Also, with different element names, it feels a bit more like different components as apposed to the same components with a different design system, at least to me.

In any case, I wanted to bring this up as I'm not sure if we've thought through these scenarios.

I think there are different ways to accomplish this. It really depends. The underlying intent here is that these are intended to be different stylesheets for the design systems. There will be different design system values as well between the FAST implementation and the MSFT implementation. This is not simply a configuration change at this point where we're swapping out the design system values - the entire system may be different for certain folks' implementations. Further, design systems apply values (such as corner radius) in different ways and based on different principles. Something like elevated-corner-radius isn't actually something that exists currently in the FAST design system. We simply have a corner-radius. Where we may want to make FAST extremely flexible and robust, it's quite possible that the MSFT design system will tighten up a bit more in an attempt to provide greater alignment with something like Fluent. It's possible over time that we will gain insight that will help us better support endless configuration of design systems easily, but we aren't there yet and I don't think it's wise for us to try and force alignment out of the gate.

I'm fine keeping this prefixed with fast-, but I'm curious if you think there are concerns with naming collision. Additional concerns are that the design systems and their default values are different, but we now have alignment in name.

To your last point - I think it's extremely important to be clear - these are different custom elements and not just a different design system configuration. If we swap the default stylesheet to align with the FAST, we cannot create exactly the MSFT styled components. Currently we would need class overrides to ensure that the components look exactly like we currently expect the MSFT components to look. There are certain things which are just going to be different, like the thumb on slider. To accomplish that with the design system, we would need to elevate a value or principle to the design system that currently doesn't really exist. That's just one example. Over time, there may be greater divergence here between systems as well.

RESOLVED: For interoperability and to ensure that there is clarity that while these may be styled differently they are a single set of components, we'll prefix with FAST.

janechu · 2020-05-13T03:27:41Z

packages/web-components/fast-components-msft/package.json

@@ -0,0 +1,47 @@
+{
+    "name": "@microsoft/fast-components-msft",
+    "description": "A library of Web Components",


Should we specify these are Microsoft styles?

…lt slot for label

nicholasrice · 2020-05-15T02:23:28Z

packages/web-components/fast-components-msft/src/design-system-provider/index.ts

+    })
+    public backgroundColor: string;
+
+    @attr({ attribute: "accent-base-color", ...fromView })


No need for the @attr or @observable anymore - see the fast-components version

eljefe223 · 2020-05-15T02:59:12Z

Smoked both packages, There seems to be a small amount of visual issues especially around focus. But I think all these issues were already there.

The only big issue I see is the tabbing behavior on the MSFT Text Field. You need to tab twice on each control, once to focus the container and again to be able to start typing

chrisdholt · 2020-05-15T03:07:39Z

Smoked both packages, There seems to be a small amount of visual issues especially around focus. But I think all these issues were already there.

The only big issue I see is the tabbing behavior on the MSFT Text Field. You need to tab twice on each control, once to focus the container and again to be able to start typing

Great catch - missing delegate focus. We'll do another run on any remaining visual issues. Well done!

* initial work to create fast-components-msft * add styles packages for patterns * add msft anchor * add msft button * add msft badge - outstanding issues * add msft card * add msft checkbox * add msft dialog * add msft divider * add msft flipper * add msft progress - do we want to narrow support to align with guidance? * add msft radio * add MSFT radio group * add msft slider and slider-label * add msft switch * add msft tabs - need to ensure alignment of tab panel with tabs * add msft text area * add msft text field * export all components from root * fix eslint errors * update imports from fast-components * add .js extension to anything distributed * update readme to include install instructions * update prefix to align with fast * update package.json description * update tsconfig * add dependency for fast-components-styles-msft * update design system provider to use MSFT design system * remove js extensions for the time being * update imports for fast-foundation * update behavior imports to correct internal path * update pathing for forcedColors behavior * fix order of imports * update system-colors path * update imports for shared utils added to foundation * fix design system signature and update to new textarea api with default slot for label * remove unused behavior and fix color on text field * update design system values and remove unused vars * fix text field focus bug

chrisdholt self-assigned this May 9, 2020

chrisdholt changed the base branch from master to features/fast-components-msft May 9, 2020 18:59

chrisdholt force-pushed the users/chhol/add-fast-components-msft-package branch 3 times, most recently from 43bd04a to 268ff8f Compare May 10, 2020 00:46

chrisdholt commented May 11, 2020

View reviewed changes

chrisdholt marked this pull request as ready for review May 11, 2020 16:34

chrisdholt requested review from awentzel, EisenbergEffect, Falkicon, janechu and nicholasrice as code owners May 11, 2020 16:34

chrisdholt commented May 11, 2020

View reviewed changes

eljefe223 approved these changes May 11, 2020

View reviewed changes

janechu reviewed May 11, 2020

View reviewed changes

packages/web-components/fast-components-msft/README.md Outdated Show resolved Hide resolved

EisenbergEffect suggested changes May 11, 2020

View reviewed changes

nicholasrice approved these changes May 11, 2020

View reviewed changes

chrisdholt force-pushed the users/chhol/add-fast-components-msft-package branch from 268ff8f to 6169d67 Compare May 12, 2020 17:46

EisenbergEffect self-requested a review May 12, 2020 17:57

chrisdholt force-pushed the users/chhol/add-fast-components-msft-package branch from aaa91e7 to d460759 Compare May 12, 2020 18:20

EisenbergEffect approved these changes May 12, 2020

View reviewed changes

janechu approved these changes May 13, 2020

View reviewed changes

chrisdholt force-pushed the users/chhol/add-fast-components-msft-package branch 3 times, most recently from 7085a9e to d464ed3 Compare May 15, 2020 00:13

chrisdholt requested a review from bheston as a code owner May 15, 2020 00:13

chrisdholt changed the base branch from features/fast-components-msft to master May 15, 2020 00:13

chrisdholt added 15 commits May 14, 2020 18:39

update readme to include install instructions

00b9a7d

update prefix to align with fast

0752b6c

update package.json description

6d53dbc

update tsconfig

08d36b0

add dependency for fast-components-styles-msft

9476101

update design system provider to use MSFT design system

b7cd0ef

remove js extensions for the time being

ecd8992

update imports for fast-foundation

c741d59

update behavior imports to correct internal path

38f9088

update pathing for forcedColors behavior

708d9cd

fix order of imports

cd2fb13

update system-colors path

09a592f

update imports for shared utils added to foundation

3741ebc

fix design system signature and update to new textarea api with defau…

5281158

…lt slot for label

remove unused behavior and fix color on text field

53b16c0

chrisdholt force-pushed the users/chhol/add-fast-components-msft-package branch from d464ed3 to 53b16c0 Compare May 15, 2020 02:00

nicholasrice reviewed May 15, 2020

View reviewed changes

update design system values and remove unused vars

a2a0b45

fix text field focus bug

053f14e

chrisdholt merged commit 0165009 into master May 15, 2020

chrisdholt deleted the users/chhol/add-fast-components-msft-package branch May 15, 2020 03:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add fast-components-msft as a new package #3096

feat: add fast-components-msft as a new package #3096

chrisdholt commented May 9, 2020 •

edited

Loading

chrisdholt May 11, 2020

eljefe223 May 11, 2020

chrisdholt May 11, 2020

eljefe223 May 11, 2020

eljefe223 May 11, 2020 •

edited

Loading

chrisdholt May 11, 2020

EisenbergEffect May 11, 2020

chrisdholt May 11, 2020

EisenbergEffect May 11, 2020

chrisdholt May 11, 2020

chrisdholt May 11, 2020

nicholasrice May 11, 2020

eljefe223 commented May 11, 2020

EisenbergEffect left a comment

EisenbergEffect May 11, 2020

chrisdholt May 11, 2020

EisenbergEffect May 11, 2020

EisenbergEffect left a comment

chrisdholt commented May 12, 2020 •

edited

Loading

janechu May 13, 2020

chrisdholt May 13, 2020

nicholasrice May 15, 2020

eljefe223 commented May 15, 2020

chrisdholt commented May 15, 2020

feat: add fast-components-msft as a new package #3096

feat: add fast-components-msft as a new package #3096

Conversation

chrisdholt commented May 9, 2020 • edited Loading

Description

Issue type checklist

Process & policy checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eljefe223 May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eljefe223 commented May 11, 2020

EisenbergEffect left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EisenbergEffect left a comment

Choose a reason for hiding this comment

chrisdholt commented May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eljefe223 commented May 15, 2020

chrisdholt commented May 15, 2020

chrisdholt commented May 9, 2020 •

edited

Loading

eljefe223 May 11, 2020 •

edited

Loading

chrisdholt commented May 12, 2020 •

edited

Loading