feat: Add Language Tools and Dictionaries page #3289

tofumatt · 2017-09-30T02:32:27Z

Fix mozilla/addons#10584.
Fix mozilla/addons#10884.

Adds a responsive table similar to the old site with language tools and dictionaries. Tried a few different UIs but this one with a responsive table actually worked pretty well so I went with it.

Screenshots

Loading Animation

tofumatt · 2017-10-05T19:36:50Z

package.json

@@ -193,7 +193,8 @@
    "react-redux": "5.0.6",
    "react-router": "2.8.1",
    "react-router-scroll": "0.4.3",
-    "react-textarea-autosize": "^5.1.0",
+    "react-super-responsive-table": "0.3.0",
+    "react-textarea-autosize": "5.1.0",


We should probably have a test for this similar to our devDependencies test that ensures we use the caret.

Oh wow, yeah. I filed https://github.com/mozilla/addons-frontend/issues/3391

tofumatt · 2017-10-05T19:39:34Z

Sorry I think this isn't quite ready for review, have to fix a bunch of flow stuff.

codecov-io · 2017-10-05T19:43:32Z

Codecov Report

Merging #3289 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    mozilla/addons-frontend#3289      +/-   ##
==========================================
+ Coverage   95.73%   95.76%   +0.02%     
==========================================
  Files         175      176       +1     
  Lines        3376     3398      +22     
  Branches      675      680       +5     
==========================================
+ Hits         3232     3254      +22     
  Misses        124      124              
  Partials       20       20

Impacted Files	Coverage Δ
src/amo/components/LanguageTools/index.js	`100% <100%> (ø)`
...rc/amo/components/AddonCompatibilityError/index.js	`100% <0%> (ø)`	⬆️
src/core/utils.js	`100% <0%> (ø)`	⬆️
src/core/api/search.js	`100% <0%> (ø)`	⬆️
src/core/searchUtils.js	`100% <0%> (ø)`	⬆️
src/core/constants.js	`100% <0%> (ø)`	⬆️
src/amo/components/Footer/index.js	`60% <0%> (ø)`	⬆️
src/amo/components/LandingPage/index.js	`100% <0%> (ø)`	⬆️
src/core/api/index.js	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 322bc00...98ff590. Read the comment docs.

tofumatt · 2017-10-06T16:20:10Z

Okay I have no idea what's happening with flow and I can't understand this output. Is it broken/buggy or just like this?

change:src/amo/components/LanguageTools/index.js
src/amo/components/LanguageTools/index.js:35
            v----------------------
 35:       {addons.map((addon) => {
 36:         return (
 37:           <li key={addon.slug}><Link to={`/addon/${addon.slug}/`}>
...:
 41:       })}
           -^ call of method `map`. Method cannot be called on possibly undefined value
 35:       {addons.map((addon) => {
            ^^^^^^ undefined

src/amo/components/LanguageTools/index.js:66
 66:       return addon.type === ADDON_TYPE_DICT || addon.type === ADDON_TYPE_LANG;
                        ^^^^ property `type`. Property not found in
 66:       return addon.type === ADDON_TYPE_DICT || addon.type === ADDON_TYPE_LANG;
                  ^^^^^ mixed

src/amo/components/LanguageTools/index.js:66
 66:       return addon.type === ADDON_TYPE_DICT || addon.type === ADDON_TYPE_LANG;
                                                          ^^^^ property `type`. Property cannot be accessed on
 66:       return addon.type === ADDON_TYPE_DICT || addon.type === ADDON_TYPE_LANG;
                                                    ^^^^^ mixed

src/amo/components/LanguageTools/index.js:75
 75:       return addon.target_locale === lang;
                        ^^^^^^^^^^^^^ property `target_locale`. Property cannot be accessed on
 75:       return addon.target_locale === lang;
                  ^^^^^ mixed

src/amo/components/LanguageTools/index.js:94
 94:                   key={addon.slug}
                                  ^^^^ property `slug`. Property cannot be accessed on
 94:                   key={addon.slug}
                            ^^^^^ mixed

src/amo/components/LanguageTools/index.js:96
 96:                   <Link to={`/addon/${addon.slug}/`}>{addon.name}</Link>
                                                 ^^^^ property `slug`. Property cannot be accessed on
 96:                   <Link to={`/addon/${addon.slug}/`}>{addon.name}</Link>
                                           ^^^^^ mixed

src/amo/components/LanguageTools/index.js:96
 96:                   <Link to={`/addon/${addon.slug}/`}>{addon.name}</Link>
                                                                 ^^^^ property `name`. Property cannot be accessed on
 96:                   <Link to={`/addon/${addon.slug}/`}>{addon.name}</Link>
                                                           ^^^^^ mixed

src/amo/components/LanguageTools/index.js:154
154:                   return addon.target_locale === langKey;
                                    ^^^^^^^^^^^^^ property `target_locale`. Property cannot be accessed on
154:                   return addon.target_locale === langKey;
                              ^^^^^ mixed

src/amo/components/LanguageTools/index.js:159
159:               if (languageTools && !toolsInLocale.length) {
                                                       ^^^^^^ property `length`. Property not found in possibly null value
159:               if (languageTools && !toolsInLocale.length) {
                                         ^^^^^^^^^^^^^ null

src/amo/components/LanguageTools/index.js:163
                                        v--------------------------------
163:               const dictionaries = toolsInLocale.filter((addon) => {
164:                 return addon.type === ADDON_TYPE_DICT;
165:               });
                   -^ call of method `filter`. Method cannot be called on possibly null value
163:               const dictionaries = toolsInLocale.filter((addon) => {
                                        ^^^^^^^^^^^^^ null

src/amo/components/LanguageTools/index.js:164
164:                 return addon.type === ADDON_TYPE_DICT;
                                  ^^^^ property `type`. Property cannot be accessed on
164:                 return addon.type === ADDON_TYPE_DICT;
                            ^^^^^ mixed

src/amo/components/LanguageTools/index.js:166
                                         v--------------------------------
166:               const languagePacks = toolsInLocale.filter((addon) => {
167:                 return addon.type === ADDON_TYPE_LANG;
168:               });
                   -^ call of method `filter`. Method cannot be called on possibly null value
166:               const languagePacks = toolsInLocale.filter((addon) => {
                                         ^^^^^^^^^^^^^ null

src/amo/components/LanguageTools/index.js:167
167:                 return addon.type === ADDON_TYPE_LANG;
                                  ^^^^ property `type`. Property cannot be accessed on
167:                 return addon.type === ADDON_TYPE_LANG;
                            ^^^^^ mixed

src/amo/components/LanguageTools/index.js:181
181:                       <LanguageToolList addons={languagePacks} /> : null}
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ React element `LanguageToolList`
 29:   addons?: Array<AddonType>,
                      ^^^^^^^^^ object type. This type is incompatible with
 52:     static values(object: any): Array<mixed>;
                                           ^^^^^ mixed. See lib: /private/tmp/flow/flowlib_38bebbe5/core.js:52

Could you have a look at these flow errors @kumar303? I am absolutely stumped and everything works fine otherwise.

kumar303 · 2017-10-06T18:47:26Z

There is one confusing error in here which is saying the type of each item in your props.addon array is mixed. It's confusing because you have specifically typed it as Array<AddonType> so each item is AddonType, not mixed.

Flow is telling you that the type definition causing the mismatch originates from here:

src/amo/components/LanguageTools/index.js:181
181:                       <LanguageToolList addons={languagePacks} /> : null}
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ React element `LanguageToolList`
 29:   addons?: Array<AddonType>,
                      ^^^^^^^^^ object type. This type is incompatible with
 52:     static values(object: any): Array<mixed>;
                                           ^^^^^ mixed. See lib: /private/tmp/flow/flowlib_38bebbe5/core.js:52

If you open up /private/tmp/flow/flowlib_38bebbe5/core.js you'll see the full type definition of Object.values() but it's wrong! This is a bug in Flow: facebook/flow#2221

You can work around it like this which isn't too bad (this makes the errors go away for me):

diff --git a/src/amo/components/LanguageTools/index.js b/src/amo/components/LanguageTools/index.js
index 4fc59c12..7d19737a 100644
--- a/src/amo/components/LanguageTools/index.js
+++ b/src/amo/components/LanguageTools/index.js
@@ -64,7 +64,8 @@ export class LanguageToolsBase extends React.Component {
   }
 
   languageTools() {
-    return Object.values(this.props.addons).filter((addon) => {
+    const { addons } = this.props;
+    return Object.keys(addons).map((key) => addons[key]).filter((addon) => {
       return addon.type === ADDON_TYPE_DICT || addon.type === ADDON_TYPE_LANG;
     });
   }

Hopefully that helps explain Flow's error output but let me know if anything else is unclear.

There are other errors but they are straight forward. Flow wants you to protect against nulls a little more directly.

I documented some debugging techniques here which is what I use when I think Flow is not inferring the correct types: https://github.com/mozilla/addons-frontend#flow

Specifically: "If you're stumped on why some Flow annotations aren't working, try using the yarn flow type-at-pos ... command to trace which types are being applied to the code. See yarn flow -- --help type-at-pos for details."

kumar303

I like how it looks but I had a few change requests.

kumar303 · 2017-10-06T19:46:41Z

package.json

@@ -193,7 +193,8 @@
    "react-redux": "5.0.6",
    "react-router": "2.8.1",
    "react-router-scroll": "0.4.3",
-    "react-textarea-autosize": "^5.1.0",
+    "react-super-responsive-table": "0.3.0",
+    "react-textarea-autosize": "5.1.0",


Oh wow, yeah. I filed https://github.com/mozilla/addons-frontend/issues/3391

kumar303 · 2017-10-06T20:12:50Z

src/amo/components/LanguageTools/index.js

+        header={i18n.gettext('Dictionaries and Language Packs')}
+      >
+        <p>
+          {i18n.gettext(`Installing a dictionary add-on will add a new languag


typo: language

kumar303 · 2017-10-06T20:13:32Z

src/amo/components/LanguageTools/index.js

+        </p>
+        <p>
+          {i18n.gettext(`Language pack add-ons change the language of your
+            Firefox browser includes menu options and settings.`)}


Can you re-word this? I'm not sure what it's saying

kumar303 · 2017-10-06T20:30:44Z

src/amo/components/LanguageTools/index.js

+  componentWillMount() {
+    const { dispatch, errorHandler } = this.props;
+
+    dispatch(fetchLanguageTools({ errorHandlerId: errorHandler.id }));


For new page loads this will make two API requests: once on the server and once on the client. A simple way to fix it would be:

export class LanguageToolsBase extends React.Component { componentWillMount() { const { addons, dispatch, errorHandler } = this.props; if (addons === null) { dispatch(fetchLanguageTools({ errorHandlerId: errorHandler.id })); } } } export const mapStateToProps = (state: Object) => { const languageToolAddons = Object.values(state.addons).filter((addon) => { return addon.type === ADDON_TYPE_DICT || addon.type === ADDON_TYPE_LANG; }); return { addons: languageToolAddons.length ? languageToolAddons : null, lang: state.api.lang, }; };

However, this state mapper may run us into performance issues eventually. A better way to do it would be to create a new addons reducer state property just for language tool add-ons. The saga would have to use a new action for loading the API response and you'd have to update the existing add-ons reducer a bit. The new property wouldn't have to be an array of add-ons, it could be an array of slugs. You would then use the existing add-ons map to fetch the objects.

Arg, good point, I totally overlooked that.

Doing this requires keying add-ons in the addons reducer state by slug, as we do elsewhere. It'll touch a lot of code I think... I'm happy to do it but I think it's a separate issue.

I'm gonna put stuff in the mapStateToProps for now, but agree this is the way to go in the future.

kumar303 · 2017-10-06T20:45:57Z

src/amo/components/LanguageTools/index.js

+
+              // This means there are no language tools available in this
+              // known locale.
+              if (languageTools && !toolsInLocale.length) {


Flow wants you to protect against nulls more directly:

if (toolsInLocale && !toolsInLocale.length) { return null; }

kumar303 · 2017-10-06T21:00:12Z

tests/unit/amo/components/TestLanguageTools.js

+    const root = renderShallow({ store });
+
+    const dictionary = root.find('.LanguageTools-in-your-locale-list-item')
+      .at(0);


I don't think it's a good idea to rely on 0 and 1 because the order is not predictable (since it relies on Object.values()). Is there another way to separate dictionaries from lang packs?

Good catch; I added a type classname to the elements to select them that way.

kumar303 · 2017-10-06T21:04:57Z

tests/unit/amo/components/TestLanguageTools.js

+      .find(LanguageToolList);
+    expect(dictionaryList).toHaveLength(1);
+    expect(languagePackList).toHaveLength(1);
+    expect(dictionaryList.shallow().find('li')).toHaveLength(1);


I think we should avoid this sort of thing -- the test is reaching into a separate component. You should be able to it in two passes:

Make sure LanguageTools is passing the right props to LanguageToolList

Assert that LanguageToolList is rendering the right thing.

tofumatt · 2017-10-10T14:02:04Z

Alright, this is all set for another review.

kumar303

r+wc

I added some straight forward change requests but let me know if you'd like another review.

kumar303 · 2017-10-10T18:35:48Z

src/amo/components/LanguageTools/index.js

+  }
+}
+
+export const mapStateToProps = (state: Object) => {


state can be typed which will help Flow provide better safety. You can write the code less defensively too.

import type { AddonState } from 'core/reducers/addons'; export const mapStateToProps = (state: {| addons: AddonState |}) => { ...

kumar303 · 2017-10-10T18:38:20Z

src/amo/components/LanguageTools/index.js

+export const mapStateToProps = (state: Object) => {
+  const { addons } = state;
+  const languageToolAddons = addons && Object.values(addons).length ?
+    Object.values(addons).filter((addon) => {


Flow complains about checking if addon.type is defined because you did not declare the type of state.addons. However, after fixing that you'll probably have to work around the Object.values() bug with:

Object.keys(addons).map((key) => addons[key]).filter(...)

Yes, this Object.values() bug is very annoying.

We could declare our own definition for Object.values() in https://github.com/mozilla/addons-frontend/tree/master/flow/libs

kumar303 · 2017-10-10T18:45:50Z

src/amo/components/LanguageTools/index.js

+    }) : null;
+
+  return {
+    addons: languageToolAddons && languageToolAddons.length ?


If the API responds with a zero length array then this will return null and create an infinite dispatch loop in componentWillMount(). It is unlikely that this will happen -- probably only with a new Docker instance of the API. The way to fix it is to make a new reducer block that responds to fetching language tools and then update the saga, etc but I know that would take a bit more time. No worries either way.

Yeah, I wanted to store languageToolsRequested as a boolean in the addons reducer state but am waiting on #3421. I've filed #3437.

kumar303 · 2017-10-10T18:51:52Z

src/amo/components/LanguageTools/styles.scss

+@import "~ui/css/vars";
+
+.LanguageTools {
+  margin: 10px;


what about 12px to align with the specs?

kumar303 · 2017-10-10T18:52:02Z

src/amo/components/LanguageTools/styles.scss

+
+  th,
+  td {
+    padding: 5px;


what about 6px to align with the specs?

tofumatt changed the title ~~feat: Add Language Tools and Dictionaries page~~ WIP: feat: Add Language Tools and Dictionaries page Sep 30, 2017

tofumatt changed the title ~~WIP: feat: Add Language Tools and Dictionaries page~~ feat: Add Language Tools and Dictionaries page Oct 5, 2017

tofumatt force-pushed the language-tools-page-2777 branch from 0305862 to db6b062 Compare October 5, 2017 19:32

tofumatt requested a review from kumar303 October 5, 2017 19:35

tofumatt commented Oct 5, 2017

View reviewed changes

tofumatt removed the request for review from kumar303 October 5, 2017 19:37

tofumatt requested a review from kumar303 October 6, 2017 16:20

eviljeff mentioned this pull request Oct 6, 2017

Add a Language Tools "Landing" page mozilla/addons#10584

Closed

5 tasks

kumar303 suggested changes Oct 6, 2017

View reviewed changes

tofumatt force-pushed the language-tools-page-2777 branch from b70963e to a3ef849 Compare October 9, 2017 16:45

tofumatt mentioned this pull request May 5, 2024

Don't store addons in top-level of addons reducer mozilla/addons#10911

Closed

tofumatt force-pushed the language-tools-page-2777 branch 2 times, most recently from 5b0973d to 96f9a8e Compare October 10, 2017 14:00

tofumatt requested a review from kumar303 October 10, 2017 14:01

kumar303 approved these changes Oct 10, 2017

View reviewed changes

tofumatt mentioned this pull request May 5, 2024

Allow zero-length language tools API response mozilla/addons#10920

Closed

feat: Add Language Tools Page (fix #2777)

98ff590

tofumatt force-pushed the language-tools-page-2777 branch from 96f9a8e to 98ff590 Compare October 11, 2017 11:47

tofumatt merged commit 1a85849 into master Oct 11, 2017

tofumatt deleted the language-tools-page-2777 branch October 11, 2017 11:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Language Tools and Dictionaries page #3289

feat: Add Language Tools and Dictionaries page #3289

tofumatt commented Sep 30, 2017 •

edited

Loading

tofumatt Oct 5, 2017

kumar303 Oct 6, 2017

tofumatt commented Oct 5, 2017

codecov-io commented Oct 5, 2017 •

edited

Loading

tofumatt commented Oct 6, 2017

kumar303 commented Oct 6, 2017 •

edited

Loading

kumar303 left a comment

kumar303 Oct 6, 2017

kumar303 Oct 6, 2017

kumar303 Oct 6, 2017

kumar303 Oct 6, 2017

tofumatt Oct 9, 2017

tofumatt Oct 9, 2017

kumar303 Oct 6, 2017

kumar303 Oct 6, 2017

tofumatt Oct 9, 2017

kumar303 Oct 6, 2017

tofumatt commented Oct 10, 2017

kumar303 left a comment

kumar303 Oct 10, 2017

kumar303 Oct 10, 2017

willdurand Oct 10, 2017 •

edited

Loading

kumar303 Oct 11, 2017

kumar303 Oct 10, 2017

tofumatt Oct 11, 2017

kumar303 Oct 10, 2017

kumar303 Oct 10, 2017

feat: Add Language Tools and Dictionaries page #3289

feat: Add Language Tools and Dictionaries page #3289

Conversation

tofumatt commented Sep 30, 2017 • edited Loading

Screenshots

Loading Animation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt commented Oct 5, 2017

codecov-io commented Oct 5, 2017 • edited Loading

Codecov Report

tofumatt commented Oct 6, 2017

kumar303 commented Oct 6, 2017 • edited Loading

kumar303 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

tofumatt commented Oct 10, 2017

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willdurand Oct 10, 2017 • 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

tofumatt commented Sep 30, 2017 •

edited

Loading

codecov-io commented Oct 5, 2017 •

edited

Loading

kumar303 commented Oct 6, 2017 •

edited

Loading

willdurand Oct 10, 2017 •

edited

Loading