Skip to content
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

Migrate additional basic components with Material 3 equivalents to googlesitekit-components-gm2 package #6113

Closed
felixarntz opened this issue Nov 5, 2022 · 20 comments
Labels
Exp: SP P1 Medium priority Sp Wk 1 Issues to be completed in the first week of the assigned sprint Type: Enhancement Improvement of an existing feature Type: Infrastructure Engineering infrastructure & tooling

Comments

@felixarntz
Copy link
Member

felixarntz commented Nov 5, 2022

See #5958 (comment):

The implementation here looks good, however taking a closer look at @material/web, there are a few more components in there which we already somewhat have equivalents for in Site Kit, which should preferably also be in this new components-gm2 package. These are:

  • Select (which we import directly from material-components) --> in GM3 it looks like this is covered by Menu (in addition to regular menus, basically it doesn't seem to make a difference between a select and a more complex menu)
  • TextField (which we import directly from material-components) --> we should create our own wrapper for it that comes with GM2 Input built-in, and then refactor the usages to rely on our new component, as this is in line with GM3 where it is a single component
  • potentially even Link --> this is part of Button in GM3 (see md-text-button)

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The CircularProgress component should be moved to the googlesitekit-components-gm2 package.
  • A new TextField component should be created in googlesitekit-components-gm2 that wraps usage of TextField, Input and HelperText from @material/react-text-field.

Implementation Brief

CircularProgress component

  • Remove usage of CircularProgress from assets/js/material-components/index.js.
  • Add it to the googlesitekit-components-gm2 module by exporting it from assets/js/googlesitekit/components-gm2/index.js and assets/js/googlesitekit-components-gm2.js.
  • Create a stub GM3 CircularProgress component that returns null, in assets/js/googlesitekit/components-gm3/CircularProgress.js, and add this to the googlesitekit-components-gm3 module.

TextField component

  • Create a GM2 TextField component within assets/js/googlesitekit/components-gm2/TextField.js, to be implemented as detailed below.
  • Add this to the googlesitekit-components-gm2 module by exporting it from assets/js/googlesitekit/components-gm2/index.js and assets/js/googlesitekit-components-gm2.js.
  • Create a stub GM3 TextField component that returns null, in assets/js/googlesitekit/components-gm3/TextField.js, and add this to the googlesitekit-components-gm3 module.
  • Remove usage of @material/react-text-field from assets/js/material-components/index.js.

Implement the GM2 version as follows:

  • The component should be a wrapper to TextField, HelperText and Input from @material/react-text-field.
    • Note that the @material/react-text-field version of TextField will be referred to as MaterialTextField in the rest of this IB to avoid confusion with the TextField wrapper we are creating.
    • Refer to existing usage of these components in the codebase for examples of their use.
  • It should accept the following props, which will be passed through to the relevant child components unless otherwise specified.
    • MaterialTextField props and their types:
      • className: string
      • name: string
      • label: string
      • noLabel: boolean
      • outlined: boolean
      • textarea: boolean
      • trailingIcon: element
      • helperText: string (this becomes the children of HelperText)
      • Note there are three props in use in the codebase that we don't actually need to provide:
        • floatingLabelClassName - we are providing the value mdc-floating-label--float-above in some cases, however this is the default and needn't be specified.
        • disabled - this is not a valid prop for MaterialTextField and doesn't do anything.
        • required - the same applies as for disabled.
    • Input props and their types:
      • id: string
      • inputType: string ('textarea', or 'input', which is the default for Input).
      • value: string - this is required, and is the only required prop.
      • size: number
      • maxLength: number
      • tabIndex: number | string
      • disabled: boolean
      • onChange: function
      • onKeyDown: function
  • Note that replacing the direct usage of the @material/react-text-field components will be done in a subsequent issue, Replace direct usage of components from @material/react-text-field with the new TextField component. #6651.

Test Coverage

  • Add JS test coverage for the TextField component using React Testing Library.

QA Brief

  • Verify the new Components/Text Fields Storybook story has Text Inputs that look and behave normally.
  • Verify the SpinnerButton Storybook story to ensure the CircularProgress works correctly.

Changelog entry

  • Migrate TextField component to our GM2 component library.
@felixarntz
Copy link
Member Author

@tofumatt @techanvil Would love to get your thoughts here.

@techanvil
Copy link
Collaborator

techanvil commented Nov 8, 2022

Hey @felixarntz, good spot here!

  • Select, agreed this should be moved to components-gm2 and implemented as a Menu / md-menu. Incidentally, unless I have missed something it appears that a Select is actually a Menu in the GM2 design language too - https://m2.material.io/components/menus#dropdown-menu

  • TextField, I like your suggestion of creating a GM2 wrapper which uses Input (also HelperText), to be migrated via md-*-text-field.

  • However when it comes to Link, I don't think we should move this to components-gm2. The current Link is not implemented as a GM2 text button, furthermore it can render using an a anchor tag for proper link semantics. Implementing as a GM3 text button would be a bit of a departure in the design. I think we should rather keep Link as it is, and use Button for GM3 text buttons. To illustrate here's the current Link in hover state, vs the md-text-button in its hover state:

image
image

  • Finally, I think we might want to add CircularProgress which is directly exported from @material-ui/core. It doesn't have a counterpoint in @material/web (at least, not at present), but it does appear to have been updated in GM3 and should presumably be part of the swappable list of components.

@techanvil techanvil removed their assignment Nov 8, 2022
@techanvil
Copy link
Collaborator

Looks like we might want to add Tab and TabBar , with corresponding md-navigation-tab and md-navigation-bar web components.

import Tab from '@material/react-tab';
import TabBar from '@material/react-tab-bar';

@bethanylang
Copy link
Collaborator

@felixarntz @tofumatt Can you please take a look at this one? This is a blocker for our next steps on the GM3 Branding Phase 1 work. Thank you!

@tofumatt
Copy link
Collaborator

tofumatt commented Feb 8, 2023

I don't think I have much to add here, I think everything's been covered by Tom.

@techanvil Would it be a good idea to make a new Button-specific issue to create that export and also update the imports? Or no.

I've added ACs (largely copied from the quoted comment) and I'll leave them with @felixarntz to review. 👍🏻

@tofumatt tofumatt assigned techanvil and unassigned tofumatt Feb 8, 2023
@techanvil
Copy link
Collaborator

@techanvil Would it be a good idea to make a new Button-specific issue to create that export and also update the imports? Or no.

Hey @tofumatt, I'm not quite sure what you mean by this point, would you mind clarifying?

@techanvil techanvil assigned tofumatt and unassigned techanvil Feb 9, 2023
@techanvil
Copy link
Collaborator

techanvil commented Feb 10, 2023

Also, a few points regarding the AC.

  • Regarding this quoted point, I'm not sure Link should be in this list, as per my comment above. Or maybe it should, but we need to be a bit clearer about how we are going to deal with it.
  • All imports of Select, Tab, TabBar, CircularProgress, and Link should be updated to use these new components imported from the googlesitekit-components-gm2 package.
  • A small point, I would have thought it would be a bit clearer to add a component called TextField rather than naming it Input seeing as it will be providing an implementation of Material's text field component.
  • Looking at the @material/web Roadmap, they are listing separate components for Menu and Select, although the Select is listed as Alpha-in-progress but I can't find it in the repo. Anyway this may have no bearing on us, or might just boil down to an implementation detail, but it's worth keeping an eye on.
  • I don't think md-navigation-tab and md-navigation-bar will be the web components to use for Tab / TabBar. That was a guess based on looking at the code, but now that the Roadmap is published, I've realised these are most likely the Navigation bar implementation, with Tabs yet to be started.

@felixarntz
Copy link
Member Author

felixarntz commented Feb 10, 2023

@tofumatt Regarding the ACs, I have nothing to add to @techanvil's feedback above, which I agree with. Let's clarify Link and I'd also agree we should use TextField as the component name if the corresponding GM3 web component is called md-text-field.

Regarding Menu and Select, if we're indeed planning to handle those separately. We already have the Menu component in components-gm2 anyway, is that part of the ACs maybe obsolete? We can probably just go with implementing a Select component now in components-gm2, and wait for the corresponding web component to become available for us to do the same for components-gm3 (in a separate issue).

Regarding Tab and TabBar, maybe let's break that part out in a separate issue as well, given there's still no clarity at this point?

@felixarntz felixarntz removed their assignment Feb 10, 2023
@tofumatt tofumatt assigned techanvil and unassigned tofumatt Feb 16, 2023
@tofumatt
Copy link
Collaborator

I think the existing Menu is quite different, but I think yes that part might be obsolete.

At this point I think @techanvil is a lot more versed in these components and would be better-suited to finished up these ACs, I think it's mostly a game of telephone from his comments to my AC-writing, but it might be better if you wrap this one up @techanvil because I think you have a lot more context than me on it! 😅

@techanvil
Copy link
Collaborator

Thanks @tofumatt, happy to pick this up and I've now updated the AC. A couple of points:

  • As @felixarntz mentioned, Menu is already in googlesitekit-components-gm2 so this point could be removed. However I've added a point to create an issue to potentially move Select into the package once we know more about the @material/web version.
  • I have left Link out of the requirements. My take is that Link is a wrapper to an anchor tag which is a foundational element that I don't see Material Design tries to avoid using. I think it's more the case that we should ensure the correct typography is applied to our links, but it's not something that has a @material/web equivalent and can be updated in-situ as needed.
    • Link can also render a button for web semantics but still styled as an anchor so I think the above still applies in this case.
    • This Link component in the mui.com component library provides a similar take: https://mui.com/material-ui/react-link/
    • We have typography for links in Figma:
      image

@techanvil techanvil assigned felixarntz and unassigned techanvil Feb 17, 2023
@felixarntz
Copy link
Member Author

@techanvil ACs LGTM, only I'm not a fan of making it part of ACs to open other issues, unless it's not clear whether those need to be opened (e.g. something needs to explored as part of IB / PR and potentially a new issue needs to be opened).

It seems to me that here we could just open those issues right away with what you have in here and start working on ACs as it is feasible (of course for the ones with external dependencies we'll need to wait).

@felixarntz felixarntz assigned techanvil and unassigned felixarntz Feb 22, 2023
@techanvil techanvil assigned felixarntz and unassigned techanvil Feb 28, 2023
@felixarntz
Copy link
Member Author

Thanks @techanvil, ACs LGTM!

@felixarntz felixarntz removed their assignment Feb 28, 2023
@techanvil techanvil self-assigned this Mar 8, 2023
@techanvil techanvil removed their assignment Mar 8, 2023
@eugene-manuilov eugene-manuilov self-assigned this Mar 22, 2023
@eugene-manuilov
Copy link
Collaborator

IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment Mar 22, 2023
@nfmohit nfmohit self-assigned this May 29, 2023
@bethanylang bethanylang added the Sp Wk 1 Issues to be completed in the first week of the assigned sprint label Jun 7, 2023
@bethanylang
Copy link
Collaborator

@nfmohit Just checking in on this one as it's blocking #6651, which is also scheduled for this sprint. Do you anticipate this will be ready for CR soon or will 6651 need to roll over? Thanks!

@nfmohit
Copy link
Collaborator

nfmohit commented Jun 13, 2023

Hi @bethanylang, thank you for checking.

I'm aiming to get this into CR by EOD today. However, #6651 will most likely still need to roll over partly.

@nfmohit nfmohit added the QA: Eng Requires specialized QA by an engineer label Jun 14, 2023
@nfmohit nfmohit removed their assignment Jun 14, 2023
@nfmohit nfmohit removed the QA: Eng Requires specialized QA by an engineer label Jun 14, 2023
@tofumatt tofumatt assigned tofumatt and nfmohit and unassigned tofumatt Jun 19, 2023
@nfmohit nfmohit assigned tofumatt and unassigned nfmohit Jun 21, 2023
tofumatt added a commit that referenced this issue Jun 21, 2023
…al-3-equivalents

Migrate Material 3 equivalents to `googlesitekit-components` package
@tofumatt tofumatt removed their assignment Jun 21, 2023
@mohitwp mohitwp self-assigned this Jun 22, 2023
@mohitwp
Copy link
Collaborator

mohitwp commented Jun 23, 2023

QA Update ✅

  • Tested on dev environment.
  • Tested Components/Text Fields and SpinnerButton storybooks.
  • Verified the new Components/Text Fields Storybook story has Text Inputs that look and behave normally.
  • Verified the SpinnerButton Storybook story to ensure the CircularProgress works correctly.

image

image

image

image

@mohitwp mohitwp removed their assignment Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Exp: SP P1 Medium priority Sp Wk 1 Issues to be completed in the first week of the assigned sprint Type: Enhancement Improvement of an existing feature Type: Infrastructure Engineering infrastructure & tooling
Projects
None yet
Development

No branches or pull requests

8 participants