feat(footer): language only for web component variations #4772

    ${useMock
      ? html`
          <dds-footer-composite
            language="${ifNonNull(language)}"
            lang-display="${ifNonNull(langDisplay)}"
            size="${ifNonNull(size)}"
+            languageList="${ifNonNull(langList)}"


Did you mean localeList? Also note that property assignment (instead of attribute assignment) takes .prop syntax.

Following the React version, I'm using the language list that was passed down to create the combo-box-items. They are different lists, unless I'm missing something..

Thanks @IgnacioBecerra for clarifying! Seems that the API in React counterpart hasn't been finalized. Started a discussion offline.

packages/web-components/src/components/footer/footer-composite.ts

packages/web-components/src/components/footer/__stories__/footer.stories.ts

asudoh · 2021-01-06T06:04:26Z

packages/web-components/src/components/footer/footer-composite.ts

+  @property({ attribute: false })
+  langList?;


Does this serve a different purpose from localeList?

Yes, it maps the combo-box-item elements to the language list that was added with this commit. The locale list has many repeated languages. See also my other comment.

packages/web-components/src/components/footer/footer.scss

packages/web-components/src/components/footer/language-selector.ts

asudoh

Thank you for the updates @IgnacioBecerra!

asudoh · 2021-01-07T01:57:31Z

packages/web-components/src/components/footer/footer-composite.ts

+        enableLanguageSelector="${enableLanguageSelector}"
+        languageSelectorLabel="${languageSelectorLabel}"
+        selectedLanguage="${selectedLanguage}"


The attribute names should be dash-cased. BTW has <dds-footer> got such attributes already?

Not originally, I added these for passing into the dds-language-selector

I'll remove these, we actually don't need them

packages/web-components/src/components/footer/footer-composite.ts

packages/web-components/src/components/footer/language-selector.ts

asudoh · 2021-01-07T02:02:38Z

packages/web-components/src/components/footer/language-selector.ts

+  /**
+   * Size property to apply different styles.
+   */
+
+  @property({ attribute: 'footer-size' })
+  footerSize = FOOTER_SIZE.REGULAR;


Does this cause any change in behavior of <dds-language-selector>?

Not at the moment, but it's for the future micro styling

Do you have some details? Wondering if we may have a better name of this property. Thanks!

The React version has a micro version of the language selector, so just like in the dds-locale-button we're storing a size property to apply the proper styles for it. The reason why it's not size like in that component is that size is another property within combo-box, hence footerSize instead of just size

I see thanks, do you have some screenshots for regular styling vs micro footer styling?

Regular styles:

Micro:

The background color, border, and position are the most prominent differences between the two. Here's the Reactversion for reference.

Thanks @IgnacioBecerra! In this case I suggest (below code should be defined in footer/defs.ts):

/** * The style scheme of language selector. */ export enum LANGUAGE_SELECTOR_STYLE_SCHEME { /** * Regular style scheme. */ REGULAR = '', /** * The style scheme that's blendid into outer style scheme. * The primary use case is for used in micro footer. */ BLENDED = 'blended', }

asudoh · 2021-01-07T02:06:12Z

packages/web-components/src/components/footer/footer-composite.ts

+  /**
+   * The placeholder for `setLanguage()` Redux action that may be mixed in.
+   *
+   * @internal
+   */
+  _languageCallback?: (e) => void;


Couldn't find the corresponding Redux action, that made me wonder what this is for...?

@IgnacioBecerra Gentle reminder - Any thoughts? Thanks!

asudoh

Thank you for the updates @IgnacioBecerra!

packages/web-components/src/components/footer/footer-composite.ts

asudoh · 2021-01-08T01:25:30Z

packages/web-components/src/components/footer/footer-composite.ts

+  @property({ attribute: 'lang-list' })
+  langList?: string;


Please see my comment to JSON.parse() code for more details:

Suggested change

@property({ attribute: 'lang-list' })

langList?: string;

@property({ attribute: false })

langList?: string[];

Setting it to string[] makes Typescript complain when trying to access its contents (text does not exist on type String), though it works anyway.

Changing it to any[] gets rid of this error.

packages/web-components/src/components/footer/__stories__/footer.stories.ts

asudoh · 2021-01-08T01:29:35Z

packages/web-components/src/components/footer/language-selector.ts

+  /**
+   * Size property to apply different styles.
+   */
+
+  @property({ attribute: 'footer-size' })
+  footerSize = FOOTER_SIZE.REGULAR;


I see thanks, do you have some screenshots for regular styling vs micro footer styling?

asudoh

Good progress @IgnacioBecerra!

packages/web-components/src/components/footer/footer-composite.ts

asudoh · 2021-01-08T03:50:07Z

packages/web-components/src/components/footer/language-selector.ts

+  /**
+   * Size property to apply different styles.
+   */
+
+  @property({ attribute: 'footer-size' })
+  footerSize = FOOTER_SIZE.REGULAR;


Thanks @IgnacioBecerra! In this case I suggest (below code should be defined in footer/defs.ts):

/** * The style scheme of language selector. */ export enum LANGUAGE_SELECTOR_STYLE_SCHEME { /** * Regular style scheme. */ REGULAR = '', /** * The style scheme that's blendid into outer style scheme. * The primary use case is for used in micro footer. */ BLENDED = 'blended', }

packages/web-components/src/components/footer/language-selector.ts

asudoh

Thank you for the update @IgnacioBecerra!

packages/web-components/src/components/footer/language-selector.ts

packages/web-components/src/components/footer/__stories__/footer.stories.ts

asudoh · 2021-01-08T06:13:39Z

packages/web-components/src/components/footer/footer-composite.ts

+  /**
+   * The placeholder for `setLanguage()` Redux action that may be mixed in.
+   *
+   * @internal
+   */
+  _languageCallback?: (e) => void;


@IgnacioBecerra Gentle reminder - Any thoughts? Thanks!

asudoh · 2021-01-08T06:15:02Z

packages/web-components/src/components/footer/language-selector.ts

+   */
+
+  @query('input')
+  private filterInputNode!: HTMLInputElement;


Please make sure private properties/methods are prefixed with _:

Suggested change

private filterInputNode!: HTMLInputElement;

private _filterInputNode!: HTMLInputElement;

asudoh · 2021-01-08T06:17:09Z

packages/web-components/src/components/footer/language-selector.ts

+  protected _handleClickOutside = e => {
+    if (!this.contains(e.target as Node)) {


Please make sure method args are type-annotated:

Style nit: Most of the codebase uses longer event variable name for event handlers. I think it's good to align to such naming strategy.

Suggested change

protected _handleClickOutside = e => {

if (!this.contains(e.target as Node)) {

protected _handleClickOutside = (event: MouseEvent) => {

if (!this.contains(event.target as Node)) {

asudoh · 2021-01-08T06:19:00Z

packages/web-components/src/components/footer/language-selector.ts

+  }
+
+  render() {
+    this._lastValidLang = this.value;


Did you mean to update _lastValidLang upon change in this.value? If so, we should use updated() lifecycle method.

asudoh

Thank you for the update @IgnacioBecerra! Getting close.

packages/web-components/src/components/footer/language-selector.ts

packages/web-components/src/components/footer/footer-composite.ts

packages/web-components/src/components/footer/language-selector.ts

asudoh

Looking good, almost there! The last style nit - Could you re-order properties/methods in the following way to align to other components?

Private/protected properties
Private/protected methods
Public properties
Public methods
Static properties

asudoh · 2021-01-08T15:53:01Z

packages/web-components/src/components/footer/language-selector.ts

+    super._handleUserInitiatedSelectItem(item);
+  }
+
+  updated(changedProperties) {


To resolve TS error observed in the CI:

Suggested change

updated(changedProperties) {

// The parent class does not define `changedProperties`

// @ts-ignore

updated(changedProperties) {

packages/web-components/src/components/footer/language-selector.ts

asudoh

LGTM 👍 - Thanks @IgnacioBecerra!

IgnacioBecerra · 2021-01-08T16:56:59Z

🙌 🎉 Thank you so much for all the guidance @asudoh!

jeffchew · 2021-01-08T17:54:49Z

Thank you @IgnacioBecerra on all of the work on this!

One thing to consider, for the documentation there isn't a clear section on how to use the language selector over the default locale button. you can see something similar in React, we may want to provide the same level of documentation:

http://ibmdotcom-react.mybluemix.net/?path=/docs/components-footer--default

jeffchew · 2021-01-08T19:56:19Z

packages/web-components/src/components/footer/__stories__/README.stories.mdx

+| Attribute                | Description                            |
+| ------------------------ | -------------------------------------- |
+| `language-selector-label`| `The input box placeholder`            |
+| `selected-language`      | `The initial selected language`        |


Just checking, isn't the localeList also necessary? Might help to also show what the expected data structure should be.

Hm, yeah I was under the impression localeList would always be passed down regardless of languageSelector being on or not for the locale-button, but that component is also optional in the first place. Let me update

temp commit

3d81568

feat(footer): language only selector introduced

5eca34f

IgnacioBecerra changed the title ~~WIP: feat(footer): language only for web component variations~~ feat(footer): language only for web component variations Jan 6, 2021

Merge branch 'master' into footer-language-only

c2363bf

IgnacioBecerra marked this pull request as ready for review January 6, 2021 05:17

asudoh reviewed Jan 6, 2021

View reviewed changes

IgnacioBecerra added 2 commits January 6, 2021 15:40

feat(footer): added fixes

4c580d7

feat(footer): ran prettier

2c3b990

asudoh reviewed Jan 7, 2021

View reviewed changes

feat(footer): addressed comments and updated snapshots

6810126

asudoh reviewed Jan 8, 2021

View reviewed changes

feat(footer): using property binding to deserialize list

83e5b2e

asudoh reviewed Jan 8, 2021

View reviewed changes

feat(language-selector): addressed more comments

9c607a4

asudoh reviewed Jan 8, 2021

View reviewed changes

IgnacioBecerra added 3 commits January 8, 2021 06:48

feat(footer): updated to conform to coding guidelines

d835127

feat(foot): removed callback

058d5fb

feat(footer): updated snapshots

4f7b782

asudoh reviewed Jan 8, 2021

View reviewed changes

IgnacioBecerra added 2 commits January 8, 2021 07:22

feat(footer): addressed feedback

0391e72

feat(footer): removed enableSelector

f3a8186

asudoh reviewed Jan 8, 2021

View reviewed changes

packages/web-components/src/components/footer/language-selector.ts Outdated Show resolved Hide resolved

IgnacioBecerra and others added 3 commits January 8, 2021 07:59

feat(footer): reorganized code

d7e678e

feat(footer): replaced updated with super call

c77c436

Merge branch 'master' into footer-language-only

a797ad5

asudoh approved these changes Jan 8, 2021

View reviewed changes

IgnacioBecerra added 2 commits January 8, 2021 10:30

chore(footer): added documentation

aa982e7

Merge branch 'master' into footer-language-only

3294e29

IgnacioBecerra added the Ready to merge Label for the pull requests that are ready to merge label Jan 8, 2021

jeffchew reviewed Jan 8, 2021

View reviewed changes

IgnacioBecerra and others added 4 commits January 8, 2021 12:10

chore(footer): updated readme

8642b95

Merge branch 'master' into footer-language-only

0679e46

Merge branch 'master' into footer-language-only

e2fe580

Merge branch 'master' into footer-language-only

ea5db15

kodiakhq bot merged commit ebf603c into carbon-design-system:master Jan 8, 2021

jeffchew mentioned this pull request Jan 19, 2021

Carbon for ibm.com Jan 2021 Release Plan (v1.15.0) #4079

Closed

	private filterInputNode!: HTMLInputElement;
	private _filterInputNode!: HTMLInputElement;

		protected _handleClickOutside = e => {
		if (!this.contains(e.target as Node)) {

feat(footer): language only for web component variations #4772

feat(footer): language only for web component variations #4772

Conversation

IgnacioBecerra commented Dec 23, 2020 • edited Loading

Related Ticket(s)

Description

Changelog

asudoh commented Dec 23, 2020

ibmdotcom-bot commented Jan 6, 2021 • edited Loading

ibmdotcom-bot commented Jan 6, 2021 • edited Loading

ibmdotcom-bot commented Jan 6, 2021 • edited Loading

asudoh 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asudoh 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

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

Choose a reason for hiding this comment

asudoh 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

asudoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asudoh 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

Choose a reason for hiding this comment

asudoh left a comment

Choose a reason for hiding this comment

asudoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asudoh left a comment

Choose a reason for hiding this comment

IgnacioBecerra commented Jan 8, 2021

jeffchew commented Jan 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgnacioBecerra commented Dec 23, 2020 •

edited

Loading

ibmdotcom-bot commented Jan 6, 2021 •

edited

Loading

ibmdotcom-bot commented Jan 6, 2021 •

edited

Loading

ibmdotcom-bot commented Jan 6, 2021 •

edited

Loading