search

illinois / queue

A microservice queue for holding open office hours
University of Illinois/NCSA Open Source License
82 stars 36 forks source link

Add last on duty staff leaving alert #173

Closed AlpriElse closed 5 years ago

AlpriElse commented 5 years ago

Added requested from Issue #146.

Added Sweet Alert library to dependencies.

capture

vercel[bot] commented 5 years ago

This pull request is automatically deployed with Now. To access deployments, click Details below or on the icon next to each push.

AlpriElse commented 5 years ago

Low key have never used a linter before today, sorry about the number of commits (didn't realize I could lint on my end and not have to rely on the pr checks).

nwalters512 commented 5 years ago

Thanks for the PR! Is there any reason you opted to pull in a new dependency versus using a Bootstrap modal? We use those pretty extensively throughout the app.

AlpriElse commented 5 years ago

I noticed you guys use modals, specifically there is component called "Confirm Modal". But, I can't pass props to change the "Are you sure message?" to something desired for this, meaning a whole new component, for a modal that's a dependency to once, would need to be made. I used Sweet Alert to make the feature addition more trivial and reduce added complexity in the project.

Just let me know if you want this reimplemented by creating a new component using modals.

nwalters512 commented 5 years ago

@AlpriElse that's just a convenience component - it's nothing more than a nice wrapper around some Bootstrap components. In the interest of keeping bundle sizes down and aiming for a consistent visual style, I'd prefer to stick with the Bootstrap modals!

Re: concerns of complexity: React components are cheap, entire new dependencies are not 🙂

AlpriElse commented 5 years ago

Reimplemented using modals. Needed to convert functional component to class to have state (following design pattern/usage of ConfirmModal).

capture