Tutorial explorer: select from list of unique org names #18290
Conversation
| @@ -0,0 +1,44 @@ | |||
| /* FilterGroupContainer: The container UI for a group of filter choices. | |||
There was a problem hiding this comment.
Components are usually .jsx files. Does it matter that this one is .js?
There was a problem hiding this comment.
.js is fine, but ideally it should be FilterGroupContainer (first letter capitalized)
There was a problem hiding this comment.
Good catch. tutorialExplorer doesn't use first letter capped at the moment but could change in a broader change later...
| } | ||
| }; | ||
|
|
||
| const FilterGroupContainer = React.createClass({ |
There was a problem hiding this comment.
In anticipation of upcoming changes to React ES6 that will no longer support .createClass, we want to use const class FilterGroupContainer extends Component and import Component like PropTypes at the top of the file. You can see an example here: #18194 Comment applies to all components in PR.
There was a problem hiding this comment.
...good point. I'd like to make this change in a separate PR for all of tutorialExplorer at once.
|
Would be nice to have a little more info in the description about what this PR is doing. |
| @@ -5,6 +5,9 @@ import * as utils from '../utils'; | |||
| // Sorting for tutorials. | |||
| export const TutorialsSortBy = utils.makeEnum('default', 'popularityrank', 'displayweight'); | |||
|
|
|||
| // Orgname value. | |||
| export const TutorialsOrgName = utils.makeEnum('all'); | |||
There was a problem hiding this comment.
Why do we have an enum with a single value? Presumably we're planning on adding more orgs?
There was a problem hiding this comment.
Not sure yet, but I did it mainly so that it'd be consistent with how TutorialsSortBy is done, above, since it works very similarly.
| getUniqueOrgNamesFromTutorials(tutorials) { | ||
| // Filter out tutorials with do-not-show tag. | ||
| const availableTutorials = tutorials.filter(t => { | ||
| return t.tags.split(',').indexOf("do-not-show") === -1 && t.orgname !== "do-not-show"; |
There was a problem hiding this comment.
Should we have a const somewhere for do-not-show along with a comment explaining what it is/how it's used?
| }); | ||
|
|
||
| // Construct array of unique org names from the tutorials. | ||
| let uniqueOrgNames = [...new Set(availableTutorials.map(t => t.orgname))]; |
There was a problem hiding this comment.
Is the goal of using Set just to get unique? If so, you could instead use _.uniq
There was a problem hiding this comment.
Yes. I thought it cool to use ES6 all the way but if we still like _ mixed in, then I can do that.
There was a problem hiding this comment.
We do generally like ES6 as much as possible, but afaik haven't used things like Set at all. I think that what you have is fine, but perhaps a little too clever. I think that your purpose (finding the unique entries) is somewhat more clear when using _.uniq 🤷♂️
|
Description updated. |
This change adds a new filter group with a single dropdown (rather than a set of checkboxes) to the Tutorial Explorer. The dropdown allows the viewer to choose an organization to see all the tutorials they've contributed, or "All" to not do any filtering based on organization. The list is sorted alphabetically, and filtered appropriately for the robotics or regular (non-robotics) variant of the page. Tutorials that are tagged "do-not-show" are not included when generating the list; likewise for organization names that are themselves "do-not-show".