Skip to content

Conversation

KatieMSB
Copy link
Collaborator

  • Identified the issue which this PR solves.
  • Read the CONTRIBUTING document.
  • Code builds clean without any errors or warnings.
  • Added appropriate tests for any new functionality.
  • All new and existing tests passed.
  • Added comments in the code, where necessary.
  • Ran make check to catch common errors. Fixed any that came up.

Description:
Added in/out transitions to the shift list for temp schedules.

@KatieMSB KatieMSB requested a review from dctalbot October 30, 2020 16:06
Copy link
Contributor

@dctalbot dctalbot left a comment

Choose a reason for hiding this comment

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

I'm seeing some odd behavior with the section headers in the shifts list when there are multiple days displayed.
Steps to reproduce:

  • create a new temp schedule for this week
  • add 4 users; 2 today, 2 tomorrow
  • delete one user from today and notice that the section header for tomorrow is duplicated for a brief moment

Copy link
Contributor

@Forfold Forfold left a comment

Choose a reason for hiding this comment

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

Have you played around with reversing the transition effect (sliding in from the right)? If so how did that experience feel?

@KatieMSB
Copy link
Collaborator Author

KatieMSB commented Nov 2, 2020

Have you played around with reversing the transition effect (sliding in from the right)? If so how did that experience feel?

Yea originally they slid in from the right, but since the fields that populate it are on the left it feels a bit better sliding from the left.

@KatieMSB KatieMSB requested review from Forfold and dctalbot November 2, 2020 20:10
Copy link
Contributor

@dctalbot dctalbot left a comment

Choose a reason for hiding this comment

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

I'm still seeing the duplicate subheader issue, only now it's just a flicker

Screen Shot 2020-11-03 at 10 43 55 AM

@KatieMSB KatieMSB requested a review from dctalbot November 3, 2020 17:30
dctalbot
dctalbot previously approved these changes Nov 3, 2020
Copy link
Contributor

@dctalbot dctalbot left a comment

Choose a reason for hiding this comment

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

I think we should be explicit about data-cy being an allowed prop. Otherwise the implicit type of ...rest is {}

Copy link
Contributor

@Forfold Forfold left a comment

Choose a reason for hiding this comment

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

looks great!

@KatieMSB KatieMSB removed the blocked label Dec 30, 2020
@KatieMSB KatieMSB merged commit 222e996 into feature/temp-schedules Dec 30, 2020
@KatieMSB KatieMSB deleted the transition-ui branch December 30, 2020 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants