-
Notifications
You must be signed in to change notification settings - Fork 1
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
APER-2852: fit and finish Skills Builder #59
Conversation
e429f09
to
8e5a38f
Compare
Codecov Report
@@ Coverage Diff @@
## main #59 +/- ##
===========================================
- Coverage 91.59% 73.46% -18.14%
===========================================
Files 30 44 +14
Lines 369 554 +185
Branches 68 119 +51
===========================================
+ Hits 338 407 +69
- Misses 30 139 +109
- Partials 1 8 +7
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
} from '@edx/paragon'; | ||
import { VisibilityFlagsContext } from './visibility-flags-context'; | ||
import { setAllFlags } from './visibility-flags-context/data/actions'; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put in a comment explaining what this component does?
const { state: visibilityFlagsState, dispatch } = useContext(VisibilityFlagsContext); | ||
const [checkboxState, setCheckboxState] = useState(visibilityFlagsState); | ||
|
||
const handleClick = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would help to explain that this handler sets the changes in the application
</Dropdown.Item> | ||
<option disabled={controlState}>{title}</option> | ||
{currentCategory.map((job, idx) => ( | ||
// eslint-disable-next-line react/no-array-index-key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this override since there IS a key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the override is saying not to use index
as the key because this is not recommended by React if the order of the items is subject to change: https://legacy.reactjs.org/docs/lists-and-keys.html#:~:text=We%20don't%20recommend%20using,an%20index%20as%20a%20key.
Since the order isn't changing, I believe it is safe to override the lint rule
8e5a38f
to
19b159e
Compare
19b159e
to
7665126
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks!
Fit and finish
Refactoring the way
useVisibilityFlags
hook is implementedThis involved creating a new context for all of the feature flags. The Skills Builder is now wrapped in this context so that every component has access to these values. This enabled us to set up the "Checkbox Feature Flag tool" (
ProductTool.jsx
in the code).To view this tool, we add
&product_tool=on
to the end of the URL for the Skills Builder.Example query string:
?product_types=boot_camp,executive_education,program,course&product_tool=on
This also works with our other query parameters such as
viz=1q
to view the single page version.Example:
?product_types=boot_camp,executive_education,program,course&viz=1q&product_tool=on
Refactoring the Categorizinator
The
CareerInterestCategorizinator.jsx
component has been refactored to useForm.Control
as a<select>
dropdown. This enabled us to have more flexibility and control over the component's state. We wanted to make sure previous selections were cleared if a different drop down was selected.single_question.mov
Error handling for when there are no results from Algolia
There was an instance that was being triggered when no results were returned for a job that was not being caught by our current error handlers. It was triggering the alert to show for the user, but then the UI would become locked in that state. Now, the alert will show and then the user can select new items to clear the error state.
Screen.Recording.2023-09-14.at.1.54.05.PM.mov