-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add ability to select multiple needs for the Add / Edit Student Modal #503
Conversation
…y -Isn't responsive enough, flashes everytime you select or deselect an option -One bug is that you can't deselect the last element
[diff-counting] Significant lines: 168. |
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.
Overall a pretty good PR and hope you are enjoying working with Typescript.
Remember to add the conditional render for the text input as well
Note that it is rendered when other
is selected so we can think of how to implement that in a generic way to support different uses of it
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.
I noticed there's quite a few hardcoded CSS px values. Its usually a more accessible choice and better for responsivity to consider using relative sizing like rem
,em
etc for windows, columns, fonts. Pixel values are best for borders and things that are not guaranteed to change.
Your margins, heights and widths should not use hardcoded pixel values. Its still mostly opinion based but my suggestion is if you stick to px values then you should add media-queries for the different device sizes
https://stackoverflow.com/questions/11799236/should-i-use-px-or-rem-value-units-in-my-css
https://wpengine.com/resources/choose-css-unit-create-better-site-layouts-how-to/#Absolute_CSS_Units
background-color: white; | ||
} | ||
|
||
.item-checkbox-selected::after { |
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.
Lets use a native HTML component for the styling of the checkbox
A comment was made in the main component about using native HTML for the checkbox
> | ||
<div className="dropdown-header-contents"> | ||
{placeHolderText} | ||
{' '} |
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.
Instead of using hardcoded whitespace
We can style the dropdown-header-contents
to have space between the placeholder and the arrow
or you can make both into a span and simply add a space between like below
<span>{placeHolderText} {isDropdownBoxOpen ? '⇈' : '⇊'}</span>
<div className="dropdown-header-contents"> | ||
{placeHolderText} | ||
{' '} | ||
{isDropdownBoxOpen ? '⇈' : '⇊'} |
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.
Lets not use hardcoded arrows like this
Check the Figma for an SVG icon for an arrow and use that
We can create one as well if necessary and simply import that from an SVGs list
|
||
{isDropdownBoxOpen && ( | ||
<> | ||
<div className="dropdown-list"> |
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.
I noticed the dropdown box was custom styled and this is okay if there is a significant change from the native HTML styling but since they are very similar (essentially the same) lets stick to using native HTML elements as they are designed to be accessible and cross platform
A suggestion is to map out everything and use input type = checkbox
Example
{isDropdownBoxOpen && (
<div className="dropdown-list">
{dataSource.map((itemName) => (
<label key={itemName} className="dropdown-list-item">
<input
type="checkbox"
checked={selectedItems.has(itemName)}
onChange={() => toggleSelectItem(itemName)}
/>
<span className="item-name">{itemName}</span>
</label>
))}
<DropdownBox | ||
placeHolderText="Select Your Needs" | ||
dataSource={Object.values(Accessibility) | ||
.splice(1) |
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.
instead of using the splice we can just remove the None value from the Accessibility enum since the stakeholders suggested there is no need for it to be there
); | ||
})} | ||
</select> | ||
Needs: |
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.
The reason why this is not bolded when the screen size is changed is because it is not wrapped using a label.
The headers for each input type uses a label and there is a media-query which keeps the label bolded on different screen sizes. Check out the code in src/components/modal/ridermodal.module.css
Needs should be wrapped like the other labels
<Label className={styles.label} htmlFor="needs">
Needs:{' '}
</Label>
…and changed styling of MultiselectBox
…MultiselectBox + ran prettier
Closing because this requires lots of backend setup before frontend can be implemented |
Summary
I added a new React component with styling called Dropdownbox. It is a dropdown checklist of items, allowing for multiple selections, with a confirm button. I also used this component in the Add / Edit Student Modal to enable the selection of multiple needs. This is purely frontend and I haven't made this multiple selection work with the backend.
When the box is not open
The box is opened and some items are selected
The box is closed
I still need to make the textbox for when "Other" is selected work with this new Dropdownbox component. My previous pull request (#497) which implemented the textbox for "Other" was done under the assumption that the select react tag was used. I also need to discuss with Lazim about possible redesigns for the needs selection because I have a different opinion on where the textbox to enter an "Other" need should be placed.
This pull request is a part of redesigning the Add / Edit Student Modal and specifically the needs selection.
Test Plan
I tested the design on different display sizes and there seems to be an issue where the "Needs: " text isn't in bold while all the other ones are.
On the Galaxy S20 inspect element
On the Responsive size in inspect element
Although none of the text are in bold when i'm just on the website and not in Inspect.
When I'm just on the website
I will try to figure out this issue in the next PRs.
Notes
The "Other" needs textbox
In the future when I incorprate the textbox for when "Other" is selected, I need to discuss with Lazim about possible redesigns. This is because implementing the textbox inside the Dropdownbox component itself will make the component not as reusable. I think that the textbox should be moved out of the dropdown checklist and be placed below it.
Lazim's design
My idea
Styling of the component
I am not very good with css, so a lot of my styling is working directly with px values which might not be good.
Behavior when click outside of the dropdown box.
I haven't implemented this later on I will make it so that the box collapses.
Breaking Changes
I haven't found any so far.