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

Implement Circular Archive date range search frontend with new DateSelectorButton #1482

Merged
merged 58 commits into from
Oct 17, 2023

Conversation

tylerbarna
Copy link
Member

@tylerbarna tylerbarna commented Oct 5, 2023

This is a PR to replace #1265 to bring it to a more recent commit and also to make use of the recent Date selector button added by @lpsinger in #1471. The appearance is now as such:

image
image

Known issues:

  • Date dropdown menu remains expanded after page is reloaded following a selection and submission of one of the date options. However, the div that contains the date range selector element is collapsed upon page submission and reloading.

  • Radio buttons are misaligned when viewed on mobile or when a desktop window is made sufficiently narrow
    image

  • Selecting an option, submitting and then switching back to the all time option before submitting again results in the button label being listed as 'undefined-now' rather than the default 'filter by date'
    image

  • When selecting a custom date range, if the start date is set, the results are cleared from the page due to the clean variable being set to false (related: clean variable in circulars index causing issues with displaying results #1110, Footer draws over floating elements #1366), resulting in the footer obscuring the end date and submit buttons
    image

  • The buttons to download circulars as a tarball are set to a higher z-index than the date range dropdown
    image

  • On mobile, most of the vertical space is occupied by the date range menu when the custom range option is selected (example is an iPhone 12 Pro)
    image

  • On mobile, smaller screens may not be able to use the date selector for the end date due to screen height limitations. This might be an issue with the DateRangeSelector element. (example is an iPhone SE)
    image

  • start and end date values are not shown in input fields after setting them and submitting them (they are set as parameters, but they are not visually shown in the input fields). Having a UseEffect set the values of the inputs results in the dates not changing visually after selecting a new date
    image

  • setting a custom date range and then submitting will result in custom range radio button being selected but the showDateRange div not being expanded
    image

  • selecting a fuzzy date (eg last year), submitting, and then switching to only submitting a custom end date will result in the startDate parameter persisting as the prior value when it should be undefined

…. This will be a new PR due to previous PR being stale
@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Attention: 57 lines in your changes are missing coverage. Please review.

Comparison is base (831baed) 5.37% compared to head (1c31122) 5.27%.
Report is 20 commits behind head on main.

❗ Current head 1c31122 differs from pull request most recent head c9266c7. Consider uploading reports for the commit c9266c7 to get more accurate results

Additional details and impacted files
@@           Coverage Diff            @@
##            main   #1482      +/-   ##
========================================
- Coverage   5.37%   5.27%   -0.11%     
========================================
  Files        118     119       +1     
  Lines       2733    2789      +56     
  Branches     285     292       +7     
========================================
  Hits         147     147              
- Misses      2584    2640      +56     
  Partials       2       2              
Files Coverage Δ
app/routes/circulars._archive._index.tsx 0.00% <0.00%> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@lpsinger lpsinger left a comment

Choose a reason for hiding this comment

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

Can you auto-submit the form if any of the fuzzy date radio buttons are clicked?

app/routes/circulars._archive._index.tsx Outdated Show resolved Hide resolved
app/routes/circulars._archive._index.tsx Outdated Show resolved Hide resolved
app/routes/circulars._archive._index.tsx Outdated Show resolved Hide resolved
app/routes/circulars._archive._index.tsx Outdated Show resolved Hide resolved
@lpsinger
Copy link
Member

lpsinger commented Oct 5, 2023

  • On mobile, smaller screens may not be able to use the date selector for the end date due to screen height limitations. This might be an issue with the DateRangeSelector element. (example is an iPhone SE)

I agree. I think that we may handle this by displaying the dropdown content as a block element on mobile and as a dropdown on desktop. But don't worry about that now; we can fix that in a future PR.

@lpsinger
Copy link
Member

lpsinger commented Oct 5, 2023

BTW, this looks very nice!

@tylerbarna
Copy link
Member Author

Can you auto-submit the form if any of the fuzzy date radio buttons are clicked?

So that was the original intent when I was originally using buttons rather than radios, but I was encountering a weird error where the Parameters were delayed in being set by one cycle (eg if on submission 1, I set the params to time A, the url would still be blank, but upon trying to set them to time B on submission 2, the url params would be set to time A). @Courey and I were unable to debug that when we initially tried, which is why I added the submit button

@lpsinger
Copy link
Member

lpsinger commented Oct 6, 2023

Can you auto-submit the form if any of the fuzzy date radio buttons are clicked?

So that was the original intent when I was originally using buttons rather than radios, but I was encountering a weird error where the Parameters were delayed in being set by one cycle (eg if on submission 1, I set the params to time A, the url would still be blank, but upon trying to set them to time B on submission 2, the url params would be set to time A). @Courey and I were unable to debug that when we initially tried, which is why I added the submit button

Alright, we can tackle that in a future PR.

tylerbarna and others added 3 commits October 9, 2023 13:35
setting button group to z-top index fixes issues regarding index for both other buttons and the footer
no longer have undefined label, will now display All Time instead
app/routes/circulars._archive._index.tsx Outdated Show resolved Hide resolved
app/routes/circulars._archive._index.tsx Outdated Show resolved Hide resolved
app/routes/circulars._archive._index.tsx Outdated Show resolved Hide resolved
removed radio button class that was not a uswds search component, removed commented out code, fixed card className, made the date range button match the labels for the radio buttons, made it so the date range filter dropdown is hidden upon submission
the radio button selected by the user will persist upon submission. Accomplished using a useEffect (thanks to Courey for the advice)
useEffect will check through the buttons and set the correct one to be checked. however, the date range div will not be expanded
@lpsinger
Copy link
Member

@tylerbarna, I have played around with this locally and I see the z-ordering glitches in the date selector that you were describing. I haven't come up with a way to fix that. I'd really like to wrap this up and free you to work on something new. Could you pare this down to a PR that just supports the fuzzy date ranges and no custom date ranges?

@tylerbarna
Copy link
Member Author

@tylerbarna, I have played around with this locally and I see the z-ordering glitches in the date selector that you were describing. I haven't come up with a way to fix that. I'd really like to wrap this up and free you to work on something new. Could you pare this down to a PR that just supports the fuzzy date ranges and no custom date ranges?

@lpsinger oh, apologies for not including this in a message, but @Courey was actually able to resolve the z-order issues I was referring to by setting the z-index to z-top for the button group. Are you referring to a different index issue?

@lpsinger
Copy link
Member

@tylerbarna, I have played around with this locally and I see the z-ordering glitches in the date selector that you were describing. I haven't come up with a way to fix that. I'd really like to wrap this up and free you to work on something new. Could you pare this down to a PR that just supports the fuzzy date ranges and no custom date ranges?

@lpsinger oh, apologies for not including this in a message, but @Courey was actually able to resolve the z-order issues I was referring to by setting the z-index to z-top for the button group. Are you referring to a different index issue?

Oh, I see! Very nice.

app/routes/circulars._archive._index.tsx Outdated Show resolved Hide resolved
app/routes/circulars._archive._index.tsx Outdated Show resolved Hide resolved
app/routes/circulars._archive._index.tsx Outdated Show resolved Hide resolved
app/routes/circulars._archive._index.tsx Outdated Show resolved Hide resolved
This refactoring will make it simpler to add SEO metadata to
routes because it will avoid having to merge meta function outputs.

According to https://remix.run/docs/en/main/route/meta#global-meta:

> Nearly every app will have global meta like the viewport and
> charSet. We recommend using normal <meta> tags inside the root
> route instead of the meta export, so you simply don't have to
> deal with merging.
- Redirect from /docs/schema to /docs/schema/stable,
  from /docs/schema/stable to /docs/schema/<latest>,
  and from /docs/schema/$version to /docs/schema/$version/gcn/notices.
- The loader for /docs/schema can now be safely cached because its
  return value does not depend on the dynamic portions of the path
  from its child routes.
- The /docs/schema/<version> index route, which was only ever used
  as a component import, is gone.
- The USWDS breadcrumb component only shows the link to the parent
  on mobile. However, our paths are never long enough that we need
  to do that. Create a new breadcrumb component that always shows
  all parts of the path.
- Remove all conditional rendering depending on screen size.
- Factor the version picker out into a separate component.
@tylerbarna
Copy link
Member Author

The 'alltime' and undefined keys in dateSelectorLabels result in a lot of branches. They are such special cases that I think it will be simpler if you remove those keys and handle them outside of the loop:

<Grid row>
  <Grid col={4}>
    <Radio label="All Time" ... />
  </Grid>
  {Object.entries(dateSelectorLabels).map(([value, label]) => ...)}
  <Grid col={4}>
    <Radio label="Custom Range..." ... />
  </Grid>
</Grid>

So I had originally set the value of the All Time radio button to be set to undefined so that it would default to displaying the all time label, but I ended up swapping the value to alltime so the radio mapping would line up; I mostly left 'undefined' there to cover any cases, but it should be fine to be removed. The custom button is actually set to the value 'custom'

I tried out removing the All Time label from dateSelectorLabels, but it causes the button labels to default to the behaviour of the parameters being two dates, so it ends up looking like:
image

the two map calls are now one. Also removed an empty className in the submit button.
@lpsinger
Copy link
Member

The 'alltime' and undefined keys in dateSelectorLabels result in a lot of branches. They are such special cases that I think it will be simpler if you remove those keys and handle them outside of the loop:

<Grid row>
  <Grid col={4}>
    <Radio label="All Time" ... />
  </Grid>
  {Object.entries(dateSelectorLabels).map(([value, label]) => ...)}
  <Grid col={4}>
    <Radio label="Custom Range..." ... />
  </Grid>
</Grid>

So I had originally set the value of the All Time radio button to be set to undefined so that it would default to displaying the all time label, but I ended up swapping the value to alltime so the radio mapping would line up; I mostly left 'undefined' there to cover any cases, but it should be fine to be removed. The custom button is actually set to the value 'custom'

I tried out removing the All Time label from dateSelectorLabels, but it causes the button labels to default to the behaviour of the parameters being two dates, so it ends up looking like: image

Probably because you also have special cases in your onClick handlers.

@tylerbarna
Copy link
Member Author

tylerbarna commented Oct 12, 2023

The 'alltime' and undefined keys in dateSelectorLabels result in a lot of branches. They are such special cases that I think it will be simpler if you remove those keys and handle them outside of the loop:

<Grid row>
  <Grid col={4}>
    <Radio label="All Time" ... />
  </Grid>
  {Object.entries(dateSelectorLabels).map(([value, label]) => ...)}
  <Grid col={4}>
    <Radio label="Custom Range..." ... />
  </Grid>
</Grid>

So I had originally set the value of the All Time radio button to be set to undefined so that it would default to displaying the all time label, but I ended up swapping the value to alltime so the radio mapping would line up; I mostly left 'undefined' there to cover any cases, but it should be fine to be removed. The custom button is actually set to the value 'custom'
I tried out removing the All Time label from dateSelectorLabels, but it causes the button labels to default to the behaviour of the parameters being two dates, so it ends up looking like: image

Probably because you also have special cases in your onClick handlers.

Isn't it because the DateSelectorButton is defined such that the startDate variable needs to exist in dateSelectorLabels in order for it to not default to displaying {startDate}-{endDate}?

{(startDate && dateSelectorLabels[startDate]) ||

@lpsinger
Copy link
Member

Isn't it because the DateSelectorButton is defined such that the startDate variable needs to exist in dateSelectorLabels in order for it to not default to displaying {startDate}-{endDate}?

Right. So for those radio buttons for which you do not want to show a custom date range, set their value to an empty string ("").

@tylerbarna
Copy link
Member Author

Isn't it because the DateSelectorButton is defined such that the startDate variable needs to exist in dateSelectorLabels in order for it to not default to displaying {startDate}-{endDate}?

Right. So for those radio buttons for which you do not want to show a custom date range, set their value to an empty string ("").

Ah, I see

going from a fuzzy date to a custom date where just the end date is set no longer has the issue of the start date still being set to the fuzzy date. Also added labels to the date range selector so that instances of selecting just a start date or just an end date will look better.
@tylerbarna tylerbarna requested a review from lpsinger October 13, 2023 19:49
@tylerbarna
Copy link
Member Author

I think the component now meets most of the specifications we were aiming for

@lpsinger
Copy link
Member

lpsinger commented Oct 16, 2023

Please rebase. There are now merge conflicts due to #1524.

@tylerbarna
Copy link
Member Author

Please rebase. There are now merge conflicts due to #1524.

should be rebased now!

app/routes/circulars._archive._index/route.tsx Outdated Show resolved Hide resolved
app/routes/circulars._archive._index/route.tsx Outdated Show resolved Hide resolved
app/routes/circulars._archive._index/route.tsx Outdated Show resolved Hide resolved
app/routes/circulars._archive._index/route.tsx Outdated Show resolved Hide resolved
app/routes/circulars._archive._index/route.tsx Outdated Show resolved Hide resolved
@tylerbarna tylerbarna requested a review from lpsinger October 17, 2023 16:30
@lpsinger lpsinger merged commit f3e18f9 into nasa-gcn:main Oct 17, 2023
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.

4 participants