Skip to content
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

feat: add allowances tab #31

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

alangsto
Copy link
Member

COSMO-366

This PR adds an additional tab to allow instructors to add exam allowances. As of now, this is not hooked up to the backend, so the tab always renders as though there are no allowances.

Screenshot 2024-07-16 at 2 27 59 PM

@alangsto alangsto marked this pull request as draft July 16, 2024 18:30
@alangsto
Copy link
Member Author

Leaving this in draft until openedx/paragon#3138 is merged. After the paragon PR is merged, I will update the package.json file, and the test failure should go away.

src/index.scss Outdated
@import "@edx/brand/paragon/variables.scss";
@import "@openedx/paragon/scss/core/core.scss";
@import "@edx/brand/paragon/overrides.scss";
@import "~@edx/brand/paragon/fonts";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this to match other MFEs.

@alangsto alangsto force-pushed the alangsto/add_allowances_tab branch from 1d20088 to 2b90f9f Compare July 16, 2024 18:38
Copy link
Contributor

@zacharis278 zacharis278 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Member

@varshamenon4 varshamenon4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome! Just a nit and a couple of questions.

src/pages/ExamsPage/messages.js Outdated Show resolved Hide resolved
@@ -25,19 +32,29 @@ const ExamsPage = ({ courseId }) => {
const {
attemptsList,
} = useExamAttemptsData();
const {
allowancesList,
} = useAllowancesData();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I like how this matches with the attemptsList above.

</Container>
<Tabs variant="tabs" mountOnEnter defaultActiveKey="attempts">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: is there a reason that the Tabs component no longer needs the defaultActiveKey?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because I modified the tabs component from an uncontrolled usage to a controlled usage (see https://paragon-openedx.netlify.app/components/tabs/). I had to do this to expose the current key, which is used to toggle whether or not the exams selector is disabled.

@@ -20,6 +21,7 @@ jest.mock('./hooks', () => ({
describe('ExamsPage', () => {
beforeAll(() => {
hooks.useExamAttemptsData.mockReturnValue(testUtils.defaultAttemptsData);
hooks.useAllowancesData.mockReturnValue({ allowancesList: [] });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: I took a look at the epic and it looks like actually having allowances is not in the scope of this work, right? That is, for now, we only need to test this case and not the case where there are allowances? Thanks for clarifying.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this ticket is only for the case where there are no allowances

href="#"
role="tab"
>
Allowances
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Is there a reason that "Allowances" isn't set as a message (for potential future internationalization, as with AllowanceList below) here? Is this just to be consistent with others (like Review Dashboard above)? Thanks for clarifying?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! I'm not entirely sure why the other tabs aren't translated, but yes, I did this to be consistent with them.

@alangsto alangsto marked this pull request as ready for review July 17, 2024 12:21
@alangsto alangsto force-pushed the alangsto/add_allowances_tab branch 4 times, most recently from 064d48c to 979eff7 Compare July 17, 2024 12:53
@alangsto alangsto force-pushed the alangsto/add_allowances_tab branch from 979eff7 to f9d3871 Compare July 17, 2024 12:54
@alangsto alangsto merged commit 8ea9bff into feature/add_allowances Jul 17, 2024
5 checks passed
@alangsto alangsto deleted the alangsto/add_allowances_tab branch July 17, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants