feat: add skills filter dropdown #76

muhammad-ammar · 2021-03-10T15:07:06Z

Description: Add a new dropdown with search capability to filter the options. The dropdown will show names of skills offered by courses.

JIRA: https://openedx.atlassian.net/browse/ENT-4104

skills_filter_v2.mov

muhammad-ammar · 2021-03-16T04:40:49Z

@adamstankiewicz This is ready for review.

raq929

I'm super excited about these changes! I would like to split things off into separate components, though, if possible.
Here's some reasoning why from a recent fedx meeting: https://openedx.atlassian.net/wiki/spaces/BPL/pages/2277081251/Paragon+Technical+Guide

I'm approving because I don't want to hold you up by being in a different time zone, but I would like smaller components and to split up the current new test, and add some others. :)

src/course-search/FacetDropdown.jsx

raq929 · 2021-03-17T17:40:22Z

src/course-search/FacetListBase.jsx

@@ -69,6 +70,8 @@ const FacetListBase = ({
      items={renderItems()}
      title={title}
      isBold={isBold}
+      attribute={attribute}
+      {...props}


I strongly disprefer passing props this way; it can make it very confusing for future people reading the code. If we know what the props are, let's pass them explicitly.

I am not sure what is the better way in this particular scenario, can you please guide?

Hmmm.... are these extra props all coming from algolia? If yes, maybe put a note in that we're passing in all the props from algolia.

mostly are coming from Algolia and some are ours.

src/course-search/tests/FacetListBase.test.jsx

raq929 · 2021-03-17T17:45:44Z

src/course-search/FacetDropdown.jsx

+        <Dropdown.Menu>
+          {
+            typeahead && (
+              <div className="search-input-wrapper">


I think there's enough difference here that we need two separate components; a TypeaheadFacetDropdown and a FacetDropdown. What they share can go in a BaseFacetDropdown component.
This will keep them easy to reason about and test.

Also, can this className go directly on the input instead of wrapping it?

Alternately, split off the inner part of this; everything within the DropdownMenu, into a TypeaheadDropdownMenu and a FacetDropdownMenu (my names are probably not the best here; feel free to do what you want with them!

src/course-search/FacetDropdown.jsx

muhammad-ammar · 2021-03-17T18:37:25Z

I'm super excited about these changes! I would like to split things off into separate components, though, if possible.
Here's some reasoning why from a recent fedx meeting: https://openedx.atlassian.net/wiki/spaces/BPL/pages/2277081251/Paragon+Technical+Guide

I'm approving because I don't want to hold you up by being in a different time zone, but I would like smaller components and to split up the current new test, and add some others. :)

@raq929 Thank You for a wonderful review. Some questions:

What do you suggest, should I merge this and refactor the code in a new PR or do the refactoring in this PR?
What is the release process? Is it documented somewhere?
What to do about the duplicate styles here in this repo and in frontend-app-learner-portal-enterprise?

raq929 · 2021-03-17T19:28:42Z

I think doing some refactoring in this PR makes sense. Hopefully it's not too big of a lift? Happy to consult, you can put time on my calendar anytime after 7:30 am
I don't think it is documented; I should make a PR checklist. It should be auto-released based on the feat: in your commit, you'll then have to make a PR on learner portal to update the version. (It should be autocreated by our bot, actually, so you just have to merge it.)
Please remove the duplicate styles in frontend-app-learner-portal-enterprise; that's my mistake in not removing them when I last updated frontend-enterprise there.

muhammad-ammar · 2021-03-19T11:42:35Z

@raq929 I have refactored the code. Please have a look. Unit test update is in progress.

raq929

I love it, it reuses code nicely, lets the FacetListDropdown not get overly complicated, and hopefully will be more straightforward to test. 🎉

muhammad-ammar · 2021-03-19T14:44:01Z

@raq929 Can you please take a final look? From my side this is ready for merge.

muhammad-ammar · 2021-03-22T16:11:44Z

@raq929 I am going merge this. Any concerns? I have tried to delete the duplicate scss files in frontend-app-learner-portal-enterprise but got into errors. I want to merge this PR and openedx/frontend-app-learner-portal-enterprise#242 as soon as possible as this is already late.

raq929

Looking good!

raq929 · 2021-03-22T17:46:07Z

package.json

@@ -95,5 +95,7 @@
    "react-instantsearch-dom": "^6.8.3",
    "react-loading-skeleton": "^2.1.1"
  },
-  "dependencies": {}
+  "dependencies": {
+    "lodash": "^4.17.21"


I switched everything to peerDependencies to avoid conflicts with the various MFEs. I think we should prooobably keep it that way for now, although we're looking at better solutions for the future.

let me look into this.

Related, do we need the full lodash dependency? Looks like it's just debounce being used; should we only require lodash.debounce as a peer dependency instead?

@raq929 This is the first time I am working with peerDependencies. As per my understanding, I need to do the below steps

move "lodash": "^4.17.21" inside peerDependencies in package.json in frontend-enterprise

remove package-lock.json changes in frontend-enterprise for this PR

add "lodash": "^4.17.21" in dependencies in package.json in frontend-app-learner-portal-enterprise

@raq929 @adamstankiewicz I have added "lodash.debounce": "^4.0.8" in package.json but git commit --amend giving me

@raq929 Can you help how to fix this peerDependencies thing and what changes I need make in this repo and in frontend-app-learner-portal-enterprise repo?

@muhammad-ammar How are you importing lodash.debounce? I'm confused why you would be seeing the linting error if you're no longer importing from the full lodash package. I'd expect the import to be something like:

import debounce from 'lodash.debounce';

@adamstankiewicz lodash.debounce issue has been fixed. Can you guide how can test styling changes for typeahead dropdown?

raq929 · 2021-03-22T17:46:25Z

src/course-search/FacetListBase.jsx

-      title={title}
-      isBold={isBold}
-    />
+    typeahead ? (


adamstankiewicz · 2021-03-22T18:01:02Z

package.json

@@ -95,5 +95,7 @@
    "react-instantsearch-dom": "^6.8.3",
    "react-loading-skeleton": "^2.1.1"
  },
-  "dependencies": {}
+  "dependencies": {
+    "lodash": "^4.17.21"


Related, do we need the full lodash dependency? Looks like it's just debounce being used; should we only require lodash.debounce as a peer dependency instead?

adamstankiewicz · 2021-03-22T18:01:44Z

src/course-search/FacetDropdown.jsx

@@ -24,10 +29,15 @@ const FacetDropdown = ({ title, items, isBold }) => (
  </div>
 );

+FacetDropdown.defaultProps = {
+  type: '',


undefined as a default prop feels a bit cleaner than an empty string here.

adamstankiewicz · 2021-03-22T18:03:44Z

src/course-search/FacetListBase.jsx

-      title={title}
-      isBold={isBold}
-    />
+    typeahead ? (


tiny nit/suggestion: Rather than using a ternary conditional here, it might be slightly more readable as 2 separate return statements?

if (typeahead) { return <TypeaheadFacetDropdown />; } return <FacetDropdown />

adamstankiewicz · 2021-03-22T18:08:14Z

src/course-search/styles/_FacetList.scss

+        padding: 5px;
+        margin: 4px;


Typically, we'd should be using the spacing tokens provided by Bootstrap and the Paragon theme for padding/margin styles, e.g., "m-2" and "p-3".

Is there an explicit need to use hardcoded spacing values here?

bad habits :(
Let me look into this.

adamstankiewicz · 2021-03-22T18:08:35Z

src/course-search/styles/_FacetList.scss

+        position: absolute;
+        top: 0;
+        left: 0;
+        width: 98%;


[curious] Why does the input only take up 98% of the width?

If we use width: 100% than focused border goes outside the area of menu. Please see the search input box inside menu in below screenshots. @adamstankiewicz can you suggest a good solution for this?

Width set to 100%

Width set to 98%

@muhammad-ammar A few things here...

.dropdown-menu { .facet-list & { padding: 50px 0 0; max-height: none; overflow: visible; width: 420px; } }

I don't believe these above styles (^^) are actually doing anything as the ordering of these nested classes are actually reversed in the DOM (i.e., .dropdown-menu is a child of .facet-list however these styles are written to expect the reverse and won't apply as a result).

You shouldn't need to modify the positioning of the the "Find a skill..." input to be absolute or even specify a width for it as it should naturally fill up all available width. The scrollable items can just naturally sit underneath the default positioning of the input.

Modifying your styles locally, I landed on the following:

.typeahead { .dropdown-menu { max-height: unset; .typeahead-dropdown-menu-scrollable-items { max-height: 250px; overflow-y: scroll; } } }

@adamstankiewicz The above styles are not working on my side. Please see the screenshot

I am not sure how to proceed on this. This is now blocked for me.

@adamstankiewicz I have checked with https://www.sassmeister.com/ and

.dropdown-menu { .facet-list & { padding: 50px 0 0; max-height: none; overflow: visible; width: 420px; } }

will be converted to

.facet-list .dropdown-menu { padding: 50px 0 0; max-height: none; overflow: visible; width: 420px; }

@muhammad-ammar, I drafted this PR (could be merged into this PR) with more concrete style suggestions to simplify things a bit here after pulling your branch down locally and testing with the local checkout of frontend-enterprise: #80

It is primarily changes in _FacetList.scss and TypeaheadFacetDropdown.jsx though there's a couple other clean up things that could happen after upgrading Paragon to latest. Largely, it removes the unneeded absolute positioning on the typeahead input element.

@adamstankiewicz Thank You! Can you share how you test/view style changes in frontend-enterprise?

@muhammad-ammar If you pull the latest from master in frontend-app-learner-portal-enterprise, you'll get some style updates that @raq929 merged today to import the styles from the frontend-enterprise package (take a look at src/index.scss to see the styles imported).

You'll also need to npm i -s lodash.debounce into frontend-app-learner-portal-enterprise so the peerDependency introduced in frontend-enterprise will resolve.

Then, checkout the branch from #80 in your local frontend-enterprise (or merge it into your branch), and use the module.config.js approach you have been doing to alias @edx/frontend-enterprise to your local checkout of the package.

You noted earlier that you need to run make build any time you make changes in frontend-enterprise. You could use the src directory instead of dist in module.config.js to get hot reloading working as you modify frontend-enterprise, e.g.:

{ moduleName: '@edx/frontend-enterprise', dir: '../frontend-enterprise', dist: 'src' },

adamstankiewicz · 2021-03-22T18:14:28Z

src/course-search/FacetListBase.jsx

@@ -84,6 +99,12 @@ FacetListBase.propTypes = {
  isCheckedField: PropTypes.string,
  items: PropTypes.arrayOf(PropTypes.shape()).isRequired,
  title: PropTypes.string.isRequired,
+  typeahead: PropTypes.shape({


nit/recommendation: I wonder if typeaheadOptions might be a bit more understandable? My first thought at the name typeahead is that it was a boolean variable, when it's actually an object.

(unless this is one of those props coming from Algolia!)

muhammad-ammar · 2021-03-23T11:28:46Z

@raq929 @adamstankiewicz I am blocked on this. I wanted to merge these yesterday no luck. There are 2 issues as we are discussing above

lodash
styling

Below is how I developed and tested all the changes.

I have below module.config.js in frontend-app-learner-portal-enterprise repo.

module.exports = {
  localModules: [
    { moduleName: '@edx/frontend-enterprise', dir: '../frontend-enterprise', dist: 'dist' },
  ],
};

I need to execute make build in frontend-enterprise everytime I make changes so that those changes are picked up by frontend-app-learner-portal-enterprise
Initially I made all the scss changes in frontend-enterprise in _FacetList.scss and _MobileSearchFilters.scss but those changes were never reflected on page so I made styling changes in frontend-app-learner-portal-enterprise as shown in feat: styles for typeahead dropdown frontend-app-learner-portal-enterprise#242 and that worked

It would be great if I can get clear guidelines to fix the above issues so that I can merge this today. Thank You!

muhammad-ammar · 2021-03-23T19:39:52Z

@adamstankiewicz Any concerns on merging this now?

muhammad-ammar · 2021-03-24T14:50:12Z

@adamstankiewicz I have cherry picked your comment. Can you give a final look at the changes especially the package.json and package-lock.json? Also what changes needs to be made in frontend-app-learner-portal-enterprise regarding lodash and frontend-enterprise?

adamstankiewicz · 2021-03-24T16:07:04Z

@adamstankiewicz I have cherry picked your comment. Can you give a final look at the changes especially the package.json and package-lock.json? Also what changes needs to be made in frontend-app-learner-portal-enterprise regarding lodash and frontend-enterprise?

package.json and package-lock.json both look good 👍

Once these changes are released to NPM, in frontend-app-learner-portal-enterprise, you'll need to upgrade frontend-enterprise to the new version and install lodash.debounce to satisfy the peer dependency.

Example: npm i -s @edx/frontend-enterprise@latest lodash.debounce

muhammad-ammar force-pushed the ammar/ent-4104-add-skill-filter-dropdown branch from abe08be to ba17276 Compare March 10, 2021 15:45

muhammad-ammar closed this Mar 10, 2021

muhammad-ammar reopened this Mar 10, 2021

muhammad-ammar force-pushed the ammar/ent-4104-add-skill-filter-dropdown branch from ba17276 to 7a6607d Compare March 11, 2021 12:49

muhammad-ammar closed this Mar 11, 2021

muhammad-ammar reopened this Mar 11, 2021

muhammad-ammar changed the title ~~WIP --- feat: add skills filter dropdown~~ feat: add skills filter dropdown Mar 11, 2021

muhammad-ammar requested a review from adamstankiewicz March 12, 2021 12:01

muhammad-ammar force-pushed the ammar/ent-4104-add-skill-filter-dropdown branch 2 times, most recently from 7e5c379 to 031f423 Compare March 15, 2021 17:01

muhammad-ammar force-pushed the ammar/ent-4104-add-skill-filter-dropdown branch from 031f423 to 213b0d0 Compare March 16, 2021 11:30

muhammad-ammar requested review from binodpant and raq929 March 17, 2021 14:39

raq929 approved these changes Mar 17, 2021

View reviewed changes

raq929 approved these changes Mar 19, 2021

View reviewed changes

muhammad-ammar force-pushed the ammar/ent-4104-add-skill-filter-dropdown branch 2 times, most recently from 3b0a6d8 to db4cd0c Compare March 19, 2021 14:40

raq929 approved these changes Mar 22, 2021

View reviewed changes

adamstankiewicz reviewed Mar 22, 2021

View reviewed changes

muhammad-ammar mentioned this pull request Mar 22, 2021

feat: styles for typeahead dropdown openedx/frontend-app-learner-portal-enterprise#242

Closed

feat: add searchable dropdown for skills filtering

f57f57d

muhammad-ammar force-pushed the ammar/ent-4104-add-skill-filter-dropdown branch from db4cd0c to f57f57d Compare March 23, 2021 19:05

fix: style fixes and misc clean up

296b1e2

muhammad-ammar force-pushed the ammar/ent-4104-add-skill-filter-dropdown branch from a5cf87e to 76eb9a8 Compare March 24, 2021 13:47

muhammad-ammar closed this Mar 24, 2021

muhammad-ammar reopened this Mar 24, 2021

muhammad-ammar force-pushed the ammar/ent-4104-add-skill-filter-dropdown branch from 76eb9a8 to fba72b9 Compare March 24, 2021 14:39

adamstankiewicz approved these changes Mar 24, 2021

View reviewed changes

fix: style and test fixes

f670c1d

muhammad-ammar force-pushed the ammar/ent-4104-add-skill-filter-dropdown branch from fba72b9 to f670c1d Compare March 24, 2021 19:33

muhammad-ammar merged commit c31b28f into master Mar 24, 2021

feat: add skills filter dropdown #76

feat: add skills filter dropdown #76

Conversation

muhammad-ammar commented Mar 10, 2021 • edited Loading

muhammad-ammar commented Mar 16, 2021

raq929 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

muhammad-ammar commented Mar 17, 2021 • edited Loading

raq929 commented Mar 17, 2021

muhammad-ammar commented Mar 19, 2021

raq929 left a comment

Choose a reason for hiding this comment

muhammad-ammar commented Mar 19, 2021 • edited Loading

muhammad-ammar commented Mar 22, 2021

raq929 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

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

Width set to 100%

Width set to 98%

adamstankiewicz Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

muhammad-ammar Mar 22, 2021 • 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

muhammad-ammar commented Mar 23, 2021

muhammad-ammar commented Mar 23, 2021

muhammad-ammar commented Mar 24, 2021

adamstankiewicz commented Mar 24, 2021

muhammad-ammar commented Mar 10, 2021 •

edited

Loading

muhammad-ammar commented Mar 17, 2021 •

edited

Loading

muhammad-ammar commented Mar 19, 2021 •

edited

Loading

Width set to `100%`

Width set to `98%`

adamstankiewicz Mar 22, 2021 •

edited

Loading

muhammad-ammar Mar 22, 2021 •

edited

Loading