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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/ timeline x days view #1885

Merged
merged 22 commits into from
Jun 22, 2022
Merged

Feat/ timeline x days view #1885

merged 22 commits into from
Jun 22, 2022

Conversation

lidord-wix
Copy link
Contributor

Timeline x days view
To see the changes, please uncomment line # 242 in the timelineCalendarScreen (and you can change it to any other number from 1 to 7)
And sorry about this big PR 馃檲
WOAUILIB-2613

Copy link
Collaborator

@ethanshar ethanshar left a comment

Choose a reason for hiding this comment

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

Looks good overall, I reviewed most of it (:
But I think it's worth finalizing the API (see my comments) before we continue.

src/timeline/Timeline.tsx Outdated Show resolved Hide resolved
src/timeline/Timeline.tsx Outdated Show resolved Hide resolved
src/timeline/Timeline.tsx Outdated Show resolved Hide resolved
src/timeline/Timeline.tsx Outdated Show resolved Hide resolved
src/timeline/Timeline.tsx Outdated Show resolved Hide resolved
src/timeline/Timeline.tsx Outdated Show resolved Hide resolved
src/timeline/Timeline.tsx Outdated Show resolved Hide resolved
src/timeline-list/useTimelinePages.ts Outdated Show resolved Hide resolved
src/expandableCalendar/style.ts Outdated Show resolved Hide resolved
@lidord-wix lidord-wix requested a review from ethanshar May 18, 2022 11:35
src/timeline/Timeline.tsx Outdated Show resolved Hide resolved
src/timeline/Timeline.tsx Outdated Show resolved Hide resolved
src/timeline/Timeline.tsx Show resolved Hide resolved
src/timeline/Timeline.tsx Outdated Show resolved Hide resolved
src/expandableCalendar/style.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@ethanshar ethanshar left a comment

Choose a reason for hiding this comment

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

See my new comments
In general notice where you create data that can affect the renders
See which data you need to memoize and which can be kept as ref

@lidord-wix lidord-wix requested a review from ethanshar May 24, 2022 14:21
@@ -44,6 +44,8 @@ export interface CalendarContextProviderProps extends ViewProps {
disabledOpacity?: number;
/** The number of days to present in the timeline calendar */
numberOfDays?: number;
/** The left inset of the timeline calendar (sidebar width), default is 72 */
leftInset?: number;
Copy link
Collaborator

@ethanshar ethanshar May 25, 2022

Choose a reason for hiding this comment

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

Does it make sense to call this prop timelineLeftInset or timelineSidebarWidth - this way the prop name also hints it's only relevant to Timeline. leftInset feels too general

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to timelineLeftInset

@@ -98,7 +98,7 @@ const headerStyleOverride = {
*/

const ExpandableCalendar = (props: ExpandableCalendarProps) => {
const {date, setDate, numberOfDays = 1} = useContext(Context);
const {date, setDate, numberOfDays = 1, leftInset = 72} = useContext(Context);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're setting the 72 default values in several places.
if we'll need to change it in the future we might miss one instance and have a bug.
Can we maybe set its default once in the context value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@lidord-wix lidord-wix requested a review from ethanshar June 7, 2022 14:25
@Inbal-Tish
Copy link
Collaborator

@ethanshar @lidord-wix Don't you want to merge this one?

@lidord-wix
Copy link
Contributor Author

lidord-wix commented Jun 8, 2022

@ethanshar @lidord-wix Don't you want to merge this one?

@Inbal-Tish no, it requires a QA session

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants