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 exam selection component #7

Merged
merged 6 commits into from
Jul 18, 2023

Conversation

varshamenon4
Copy link
Member

@varshamenon4 varshamenon4 force-pushed the varshamenon4/exam-selection-component branch from 3e20d3d to 6cf52e0 Compare June 26, 2023 20:46
@varshamenon4 varshamenon4 force-pushed the varshamenon4/exam-selection-component branch from d7d864b to 91101a6 Compare July 6, 2023 23:25
@varshamenon4 varshamenon4 force-pushed the varshamenon4/exam-selection-component branch from da5a6fd to 8a3dc5f Compare July 7, 2023 13:47
Copy link
Member Author

@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.

Here's where I've left off! Added some comments.

src/pages/ExamsPage/components/ExamSelection.jsx Outdated Show resolved Hide resolved
src/pages/ExamsPage/data/reducer.js Outdated Show resolved Hide resolved
src/pages/ExamsPage/hooks.js Outdated Show resolved Hide resolved
Zacharis278
Zacharis278 previously approved these changes Jul 7, 2023
src/pages/ExamsPage/data/reducer.js Outdated Show resolved Hide resolved
src/pages/ExamsPage/components/ExamSelection.jsx Outdated Show resolved Hide resolved
src/pages/ExamsPage/hooks.js Outdated Show resolved Hide resolved
@Zacharis278 Zacharis278 dismissed their stale review July 10, 2023 13:19

should have been comment

border: 1px solid $search-border-color !important;
}

.pgn__searchfield.has-focus:not(.pgn__searchfield--external)::after {
Copy link
Member

Choose a reason for hiding this comment

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

override some of the default styling to deal with putting searchfield inside of a select dropdown. The default focus border looks odd here and causes sizing issues so I've removed it and placed a simple border on the input element instead.

Copy link
Member

@alangsto alangsto left a comment

Choose a reason for hiding this comment

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

A few questions about messages/internationalization

const { formatMessage } = useIntl();
const [searchText, setSearchText] = useState('');

const placeholderMessage = formatMessage({
Copy link
Member

Choose a reason for hiding this comment

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

could these messages be moved to a message specific file? I typically haven't seen them defined within the components.

Copy link
Member

Choose a reason for hiding this comment

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

Also are we considering internationalization?

Copy link
Member

Choose a reason for hiding this comment

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

oh yeah you're right, this should be done using a separate file.

Copy link
Member

Choose a reason for hiding this comment

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

ok this should be fixed. Turns out I had already set that file up I just didn't make use of it.

@Zacharis278 Zacharis278 marked this pull request as ready for review July 18, 2023 17:17
@Zacharis278 Zacharis278 force-pushed the varshamenon4/exam-selection-component branch from 935cece to 52d31fc Compare July 18, 2023 18:04
Copy link
Member

@alangsto alangsto left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Zacharis278 Zacharis278 merged commit 662f6f5 into main Jul 18, 2023
2 checks passed
@Zacharis278 Zacharis278 deleted the varshamenon4/exam-selection-component branch July 18, 2023 18:47
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