Skip to content
This repository has been archived by the owner on Feb 18, 2022. It is now read-only.

use opacity on datepicker show/hide #466

Closed
wants to merge 1 commit into from

Conversation

glen0071
Copy link
Contributor

@glen0071 glen0071 commented May 21, 2020

Description of the change

DatePicker selection not saving on Firefox.

DatePicker component with .react-date-picker__calendar--closed class was set to display: none. In Firefox, maybe element is removed from DOM before onChange method triggers?

Using opacity to conceal/show DatePicker seems to work on Firefox (Chrome worked either way - even without InputDate.css rules on closed/open, I guess Chrome works well with react-date-picker's out-of-the-box styling).

In giph, Firefox on left, Chrome on right
fix-datepicker

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Configuration change (adjusts configuration to achieve some end related to functionality, development, performance, or security)

Related issues

Closes #259

Checklists

Development

These boxes should be checked by the submitter prior to merging:

  • Manual testing has been performed locally

Code review

These boxes should be checked by reviewers prior to merging:

  • This pull request has a descriptive title and information useful to a reviewer
  • This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
  • Potential security implications or infrastructural changes have been considered, if relevant

@vercel
Copy link

vercel bot commented May 21, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/recidiviz/covid19-dashboard/ai9td2j57
✅ Preview: https://covid19-dashboard-git-eds-259-date-picker-bug.recidiviz.now.sh

@glen0071 glen0071 requested a review from jessex May 21, 2020 04:07
display: none;
opacity: 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is mysterious to me since the date picker still works in Firefox in other places (e.g. the add cases modal), but if it does fix the issue I don't necessarily have an issue with it. However, one note - because this CSS file is copied out of the package to work around a bug (see comment at the top) I would rather we didn't make the change here, but rather did an override in the styled component inside DatePicker.tsx instead.

We might not even need this file anymore because it was an issue specific to Parcel, so I wouldn't want us to cause a regression when we get around to removing it.

Copy link
Member

@jessex jessex left a comment

Choose a reason for hiding this comment

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

Unfortunately, I think this approach either masks the issue or creates another one. Now, even if you click out of the date picker or make a selection, it is still an interactive item.

For example: Open the add cases modal, select a date, see that the the date picker is no longer visible, but see that your pointer will still be the active clicky hand, and if you click around in the vicinity of where the date picker was, you will notice that the date will change in the actual text box--the date picker is still active and taking input, but it is now invisible.

I see this happening with the date picker in both the Add Cases modal and the Future Release feature, so I assume this is impacting all date pickers in the app. Furthermore, I see this happening in both Firefox and Chrome, so it appears to impact all browsers. I confirmed this is not happening in production.

@glen0071
Copy link
Contributor Author

Ok. I'm going to dig into this some more and find a better fix for it. And implement it in the DatePicker's styled components.

@glen0071
Copy link
Contributor Author

@jessex @macfarlandian
Seems like react-date-picker does not work with GatsbyJS. After ruling out a lot of other possibilities, I spun up a starter Gatsby app and plopped react-date-picker into the middle it. Same issue in Firefox.

I think it works on the AddCasesModal because the Modal attaches to the DOM independent from the rest of the app.

Not sure if we can configure Gatsby to fix this. Should I adapt the InputDate component (which AddCasesModal also relies on) to attach to the DOM?

@macfarlandian
Copy link
Collaborator

@glen0071 I'm not sure I totally follow the logic ... both the modal and the form page are React components and therefore part of the Gatsby app. I'm not seeing the causal link here, maybe you can explain further?

@glen0071
Copy link
Contributor Author

glen0071 commented May 26, 2020

@macfarlandian Good point. I was probably over confident in that last comment. FWIW, I tried adding/removing/duplicating different components in different combinations. The only thing that seemed to have an affect in Firefox is when the reac-date-picker was in a component mounted to the DOM via

return ReactDOM.createPortal(
    <BackgroundAside onClick={handleOnClick}>
      <ModalContainer ref={ref} height={height} width={width}>
        <ModalTitle title={title} closeModal={closeModal} />
        {children}
      </ModalContainer>
    </BackgroundAside>,
    document.getElementById("___gatsby") as HTMLElement,
  );

So, I have to admin I don't know why that is or what that means. I can continue thinking/digging down that path. It made me feel like the easiest fix might be to attach the react-date-picker's parent component to some DOM id. But I'd love to hear other ideas, or else I'll dig around a bit later tonight.

@macfarlandian
Copy link
Collaborator

that's interesting ... I guess I would feel better if we could identify a root cause ... that still just feels like a mysterious and possibly brittle workaround to me

@glen0071
Copy link
Contributor Author

glen0071 commented May 27, 2020

@macfarlandian I've done some more digging, but not much progress.

ReactDOM docs sorta clarify why that can fix it:

The react-dom package provides DOM-specific methods that can be used at the top level of your app and as an escape hatch to get outside of the React model if you need to. Most of your components should not need to use this module.

We know it doesn't work, and it doesn't throw any errors in Firefox/Safari. And since the same problem arises in a brand new Gatsby project, but not a create-react-app project, I suspect it's something unique to react-date-picker + Gatsby + Firefox/Safari.

To find a root cause, I guess the next step is to dig into react-date-picker's implementation for clues? Or focus on Gatsby docs? (I fished around there a bit, but I don't think I saw anything relevant, so I thought react-date-picker source code might be a better route.)

@macfarlandian
Copy link
Collaborator

@glen0071 yeah probably react-date-picker ... if you can identify a bug in there they might just fix it, I believe it is actively maintained

@glen0071
Copy link
Contributor Author

glen0071 commented Jun 5, 2020

@macfarlandian I opened an issue on react-date-picker, hoping to get some input from its author on how to fix this. Also wondering if it's best to close this PR since the fix will probably come from that library?

@macfarlandian
Copy link
Collaborator

@glen0071 yeah I think it's fine to close this ... might be good to note the related issue on react-date-picker in our ticket so we can follow up later

@glen0071
Copy link
Contributor Author

glen0071 commented Jun 5, 2020

Opened an issue on the react-date-picker package, wojtekmaj/react-date-picker#274

@glen0071 glen0071 closed this Jun 5, 2020
@glen0071 glen0071 deleted the eds/259-date-picker-bug branch June 5, 2020 17:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Firefox: Date picker in model input is broken
3 participants