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

Add wildcard/calendar component based on 'react-calendar' npm package #40679

Merged
merged 5 commits into from
Aug 24, 2022

Conversation

erzhtor
Copy link
Contributor

@erzhtor erzhtor commented Aug 22, 2022

Part 1 of #40593.

For the new user-management page, we need to add column filters, and some columns are of date type. This PR adds a Calendar component based on react-calendar, with basic Sourcegraph styles (light/dark mode) that supports date or date range selection.

Note: there are not many existing react library options that would be simple and yet allow us fully style it.

@sourcegraph/frontend-platform, @rrhyne I thought that this base Calendar could be useful to extract into wildcard in case there will be some other places where a calendar or date range picker will be needed. However, if you think this should not be included in the wildcard yet, I can close this PR and just include it directly in the user-management page folder.

Test plan

Screenshots

Description screenshot
single date select mode image
<Calendar value={value as [Date, Date]} onChange={onChange} />
date range select mode image
<Calendar mode="range" value={value as [Date, Date]} onChange={onChange} />
dark mode image

App preview:

Check out the client app preview documentation to learn more.

@erzhtor erzhtor requested review from rrhyne, ryphil, thenamankumar and a team August 22, 2022 15:26
@erzhtor erzhtor self-assigned this Aug 22, 2022
@cla-bot cla-bot bot added the cla-signed label Aug 22, 2022
@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Aug 22, 2022

Codenotify: Notifying subscribers in OWNERS files for diff 7d48b11...ca54aea.

Notify File(s)
@sourcegraph/frontend-platform-devs client/wildcard/src/components/Calendar/Calendar.module.scss
client/wildcard/src/components/Calendar/Calendar.story.tsx
client/wildcard/src/components/Calendar/Calendar.tsx
client/wildcard/src/components/Calendar/index.ts
client/wildcard/src/components/index.ts

@erzhtor erzhtor force-pushed the erzhtor/add-wildcard-calendar-component branch from ef9b45d to 2080f71 Compare August 23, 2022 03:56
@lrhacker
Copy link
Contributor

Hey @erzhtor! Thank you for just getting in there and putting this together 😎.

As for adding the component to Wildcard (or not), these are the options @sourcegraph/frontend-platform team would be fine with:

Option 1: Since this component is brand new to the codebase (versus moving an existing component with a history of use to Wildcard), it can start in Wildcard if it gets annotated via JSDoc comments that include:

  • Any information/context that is important to understand when using the component (see the newer Tooltip component for an example of this)
  • A note that it is still an "experimental" component (in this case, meaning it is still newly added to the codebase and may still have some issues to work out if folks try and use it in other locations - there would also be no guarantee for immediate support to resolve these issues from Frontend Platform Team, given current priorities and timelines)

Option 2: Skip adding to Wildcard for now and include it directly in the user-management page folder, as you mentioned in your original comment.

For either of these options, we can consider the Calendar component when we get to planning Wildcard v3 later this year (whether that's moving the component out of an experimental state and committing to support it, or moving it into Wildcard if you opt not to add it there now).


export default config

// NOTE: hardcoded in order to screenshot test the calendar
Copy link
Member

Choose a reason for hiding this comment

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

🙏

Copy link
Member

@valerybugakov valerybugakov left a comment

Choose a reason for hiding this comment

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

Minor accessibility issue: the contrast of the highlighted today's date is not spec compliant and hard to read when selected in the dark mode.

Screenshot 2022-08-24 at 20 18 27

Tested using the deployed version of the Storybook.

My vote here goes for the first option: add the experimental label and any other context required to the component usage (basic docs) and add it to the Wildcard. Thanks for tagging us here and for the intent to help other teams with this component! 🙌

client/wildcard/src/components/Calendar/Calendar.tsx Outdated Show resolved Hide resolved
client/wildcard/src/components/Calendar/Calendar.tsx Outdated Show resolved Hide resolved
client/wildcard/src/components/Calendar/Calendar.tsx Outdated Show resolved Hide resolved
@erzhtor
Copy link
Contributor Author

erzhtor commented Aug 24, 2022

Thanks, @valerybugakov, @lrhacker for the input and reviews! I've addressed comments and added jsDocs. Also added messaging to mention the "experimental" state of the component to both jsDocs as well as on storybook stories (to make it explicit to other devs).

@erzhtor erzhtor force-pushed the erzhtor/add-wildcard-calendar-component branch from a3c0e9c to ca54aea Compare August 24, 2022 15:06
@erzhtor erzhtor merged commit 93341e0 into main Aug 24, 2022
@erzhtor erzhtor deleted the erzhtor/add-wildcard-calendar-component branch August 24, 2022 15:25
@vovakulikov
Copy link
Contributor

@erzhtor why do we need a custom calendar picker component? (In general I have nothing against custom pickers, but just try to play a devil's advocate here)
My consers about this custom picker

  • Usually custom pickers are not fully accessible on mobiles (compared to native date pickers)
  • React-calendar declares that this is a lightweight solution but at the first glance it seems that this brings another 58kb in minified version in our bundle (maybe this is a false concern because I see some disputes about bundlephobia size measurements for this lib)
  • This is another way to do something about dates. Native pickers should be easier to learn (IMO) on how to use them.

Maybe we could push back on designs about the site admin pages where we have to have this custom date picker in order to avoid these possible problems that I list above.

@erzhtor
Copy link
Contributor Author

erzhtor commented Sep 1, 2022

@erzhtor why do we need a custom calendar picker component? (In general I have nothing against custom pickers, but just try to play a devil's advocate here) My consers about this custom picker

  • Usually custom pickers are not fully accessible on mobiles (compared to native date pickers)
  • React-calendar declares that this is a lightweight solution but at the first glance it seems that this brings another 58kb in minified version in our bundle (maybe this is a false concern because I see some disputes about bundlephobia size measurements for this lib)
  • This is another way to do something about dates. Native pickers should be easier to learn (IMO) on how to use them.

Maybe we could push back on designs about the site admin pages where we have to have this custom date picker in order to avoid these possible problems that I list above.

Hey, @vovakulikov. Thanks for bringing up your concerns. All of the above said, I think is valid. However, for "site-admin / user administration," we have to make a custom date range picker with additional custom logic:
image.
What I can suggest (and do myself) is to move this Calendar component from wildcard back to "user administration" dir so that it is not used in other places.

However, I don't think we should be changing the current date range selected in "user administration" to native date inputs because:

  1. Native date inputs are quite not good for date range selects. I believe we have to put two inputs to capture the date range.
  2. I don't think that admins use site admin on mobile phones. Even the current version is not responsive and doesn't fit mobile screens.
  3. The user administration is still early, so it might change based on feedback once admins try it. This means that time spent on redoing the work might be wasted if we don't keep it in the long term.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants