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

date picker #68

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

date picker #68

wants to merge 11 commits into from

Conversation

alissav30
Copy link
Collaborator

Fixed the Date Picker issue we were having

commit c123743
Author: pjames27 <[email protected]>
Date:   Thu Apr 11 08:02:25 2024 -0700

    Fixed wording on waiver

commit 0c2d0de
Author: pjames27 <[email protected]>
Date:   Thu Apr 11 00:40:53 2024 -0700

    Resolved #53

commit 180bd45
Author: pjames27 <[email protected]>
Date:   Thu Apr 11 00:25:39 2024 -0700

    Resolved #21

    This commit is a doozy - definitely look at all changed files here.

commit 1ef1f7b
Author: pjames27 <[email protected]>
Date:   Wed Apr 10 23:41:33 2024 -0700

    Fixed Cardinal Care encoding according to Abhinav's update

    - Undid a rule removal from my last commit

commit d45dc16
Author: pjames27 <[email protected]>
Date:   Wed Apr 10 22:04:41 2024 -0700

    Many changes related to #42 and #51

    - Removed rules from the ruleset that resulted in the covid-specific rules being ignored.
    - Updated the facility label to be "Outpatient Center", since Hospital already appeared.
    - Updated the tour dataset to correctly pass for covid vaccines
    - Resolved #42

commit 142bbd8
Author: pjames27 <[email protected]>
Date:   Wed Apr 10 21:03:44 2024 -0700

    Resolved #51

commit bd5a250
Author: pjames27 <[email protected]>
Date:   Wed Apr 10 20:34:46 2024 -0700

    Resolved #55

commit 5419eba
Author: pjames27 <[email protected]>
Date:   Wed Apr 10 20:13:31 2024 -0700

    Resolved #57

commit cbfb491
Author: pjames27 <[email protected]>
Date:   Wed Apr 10 20:06:06 2024 -0700

    Resolved #52

commit 7c9a343
Author: pjames27 <[email protected]>
Date:   Wed Apr 10 17:24:20 2024 -0700

    Resolved #56

commit 9de5471
Author: pjames27 <[email protected]>
Date:   Wed Apr 10 16:56:00 2024 -0700

    Resolved #54

commit 2155cc8
Author: pjames27 <[email protected]>
Date:   Wed Apr 10 16:27:30 2024 -0700

    Fixed #49

commit 076d6d3
Merge: af4204d dd7b7d5
Author: pjames27 <[email protected]>
Date:   Wed Apr 10 12:09:13 2024 -0700

    Merge branch 'alissa-branch'

commit af4204d
Author: pjames27 <[email protected]>
Date:   Wed Apr 10 09:54:47 2024 -0700

    Resolved #16

commit 2a88ab8
Author: pjames27 <[email protected]>
Date:   Wed Apr 10 09:27:15 2024 -0700

    Resolved #34

commit 768afe2
Merge: 97e0046 4327cb6
Author: pjames27 <[email protected]>
Date:   Wed Apr 10 09:13:04 2024 -0700

    Merge branch 'alissa-branch'

commit 97e0046
Author: pjames27 <[email protected]>
Date:   Wed Apr 10 09:08:25 2024 -0700

    Updated waiver styling

commit f778613
Merge: f6bb071 96991e4
Author: pjames27 <[email protected]>
Date:   Wed Apr 10 08:38:02 2024 -0700

    Merge pull request #45 from pfw13/alissa-branch

    finished the "waive cardinal care"page

commit f6bb071
Author: pjames27 <[email protected]>
Date:   Wed Apr 10 01:53:19 2024 -0700

    Resolved #44
so that the styling doesn't look awkward on the dropdwon
@alissav30 alissav30 requested a review from pfw13 April 29, 2024 21:41
alissav30 and others added 4 commits May 6, 2024 14:33
lost some changes when i merged main into this branch so just restoring date picker fixes i had made
onChange={handleChange}
onBlur={onBlur}
className={COMMON_INPUT_CLASSES + " pl-4 pr-2 py-2 border rounded"}
style={{ display: 'block', width: '100%', fontSize: '16px', lineHeight: '20px' }}
Copy link
Collaborator

@pfw13 pfw13 May 20, 2024

Choose a reason for hiding this comment

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

I would advise against initializing objects directly in the properties. To determine if a component should be re-rendered, React compares what was inputted as a property against the old value for equality using the Object.is() comparison function. Since objects are passed by reference in JS, the "old" { display: 'block', width: '100%', fontSize: '16px', lineHeight: '20px' } will never equal the "new" { display: 'block', width: '100%', fontSize: '16px', lineHeight: '20px' }.

I also did this sometimes to progress quickly, but in case you weren't aware of this, I just wanted to bring this up.

? value.some((v) => v.id === opt.id)
: value && value.id === opt.id;
const isSelected = (opt: TOptions[number]) => {
const optionKey = opt.id || opt.value; // Assuming all options have either an id or value
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a need to not force every option to have an ID and use this?

onChange?.(
(Array.isArray(value)
{options.map((opt) => {
const optKey = opt.id || opt.value; // Handle options with id or value
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above

"bg-blue-200 hover:bg-blue-100",
])}
disabled={opt.specialStatus === "disabled"}
onClick={() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should move this outside of the return statement into a useCallback hook.

formValuesToEpilog: (values) => {
console.log("values",values);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably it's a good idea to be more specific with what exactly we are logging here. Otherwise, it's hard for other developers (or even your future you) to derive meaning from this.

/* --------------------------------- Person --------------------------------- */

if (!values.person) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really want the program to crash if no person or policy was provided? At the moment, this seems to me to force the user to fill out these fields before he is able to save the form. Is that expected beahvior?

Copy link
Collaborator

@pfw13 pfw13 left a comment

Choose a reason for hiding this comment

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

I don't know if it's a good idea to replace Explore.tsx with the new one before we've made sure that the new version is running without bugs. If that is not the case by now, I suggest to rename Explore_old.tsx to Explore.tsx again and rename Explore.tsx to Explore_new.tsx. Let's discuss that at our meeting later.

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