refactor Event Schedule UI for new design + add personal schedule #1302

igachmovmelon · 2021-04-02T12:08:29Z

closes https://github.com/sparkletown/internal-sparkle-issues/issues/485

Removed LiveSchedule component as it's no longer used a6cd04b
Removed SchedulePage component as its' no longer used a6cd04b

Preview:

Feature structure:

SchedulePageModal                           <---  I suggest renaming it to ScheduleModal
├── ScheduleVenueDescription
├── Schedule
       ├── rooms 
       ├── ScheduleRoomEvents
               ├── ScheduleEvent

SchedulePageModal is the main component which appears and hides on Schedule button click.
ScheduleVenueDescription contains general information about the parent (if exists) or current venue.
Schedule the component which contains all the information about events of the selected day.
ScheduleRoomEvents is just a map of events in the room line.
ScheduleEvent is the component of the particular event.

There is a component EventDisplay which was modified by Ivan (ScheduleEvent in my case). But it's used in several places and I'm not sure if it was safe to modify it. If we don't have anything schedule-related in the project, we might want to remove that piece as well. Open to your thoughts.

codeclimate · 2021-04-02T12:09:09Z

Code Climate has analyzed commit df146ea and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1
Duplication	2

View more on Code Climate.

src/components/molecules/EventRoomDisplay/EventRoomDisplay.tsx

github-actions · 2021-04-02T12:13:20Z

Visit the preview URL for this PR (updated for commit df146ea):

https://co-reality-staging--preview-pr-1302-dca4vx19.web.app

_{(expires Tue, 01 Jun 2021 13:51:28 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

0xdevalias · 2021-04-07T00:22:02Z

@mike-lvov Should this branch be targeting staging as our new canonical schedule, or is it going to be merged directly into the custom environment?

mike-lvov · 2021-04-07T11:35:02Z

@0xdevalias yes, it should be applied to all of the envs. Synced with @mobilevinay on this one.

src/components/molecules/EventDisplay/EventDisplay.tsx

src/utils/time.ts

mike-lvov

Looks valid. LGTM to merge in. A few non blocking comments to fix after it is merged

src/components/molecules/EventPaymentButton/EventPaymentButton.tsx

src/components/molecules/Schedule/Schedule.utils.ts

mike-lvov · 2021-05-17T21:34:52Z

src/components/molecules/ScheduleEvent/ScheduleEvent.scss

+$ScheduleEvent--margin-top: 0.25rem;
+$ScheduleEvent--height: 3.75rem;
+$ScheduleEvent--padding: 0 0.25rem 0 0.75rem;
+$ScheduleEvent--border-radius: 18px;
+$ScheduleEvent--box-shadow: 0 5px 10px rgba(0, 0, 0, 0.65);
+
+$bookmark--padding: 0.5rem;
+$bookmark-hover--padding-top: 0.375rem;


[NB] Any reasons to have them in rem?

Ref:

https://developer.mozilla.org/en-US/docs/Learn/CSS/Building_blocks/Values_and_units#relative_length_units

Relative length units are relative to something else, perhaps the size of the parent element's font, or the size of the viewport. The benefit of using relative units is that with some careful planning you can make it so the size of text or other element scales relative to everything else on the page.

rem: Font size of the root element.

mike-lvov · 2021-05-17T21:35:38Z

src/components/molecules/ScheduleEvent/ScheduleEvent.scss

+  }
+
+  &__info {
+    font-size: 0.8rem;


[NB] use fontsize from const. 0,8 * 16 =~ 12

@mike-lvov Is the const in px? Moving to using em/rem across the codebase is probably a better overall solution for font-size than using scss const's I would think?

See docs link in #1302 (comment)

@0xdevalias I don't mind moving to em/rem eventually. What I want to have is consistency & to achieve it, we should use the defined font-size consts(which are currently defined in px, but can be changed to rem/em in 5 lines of code, which would apply across the app)

Yeah, fair enough, makes sense.

mike-lvov · 2021-05-17T21:36:04Z

src/components/molecules/ScheduleEvent/ScheduleEvent.scss

+    text-overflow: ellipsis;
+    display: block;
+    margin-top: $description--margin-top;
+    font-size: 0.7rem;


[NB] Same as above ^

See my comments in #1302 (comment)

#1302 (comment)

not sure what to do with this one.
It should be less than 12px (event title), and we have $font-size--xs: 10px, but IMO it's too small.

0.8 rem + 0.7 rem

12px + 10px (host label is more difficult to read)

Should I create a new constant then (11px)? And how should I name it?
xs = 11px
xxs = 10px?

current list:

$font-size--xs: 10px; $font-size--sm: 12px; $font-size--md: 14px; $font-size--lg: 16px; $font-size--xl: 20px; // Not sure this is the best naming for these $font-size--xxl: 30px; $font-size--xxxl: 50px;

@mike-lvov Thoughts?

src/components/molecules/ScheduleEvent/ScheduleEvent.tsx

src/components/molecules/ScheduleVenueDescription/ScheduleVenueDescription.scss

mike-lvov · 2021-05-17T21:38:30Z

src/components/molecules/ScheduleVenueDescription/ScheduleVenueDescription.scss

+  }
+
+  &__desc {
+    font-size: 0.8rem;


[NB] use from constants

See my comments in #1302 (comment)

src/components/molecules/ScheduleVenueDescription/ScheduleVenueDescription.scss

src/components/organisms/NavBarSchedule/NavBarSchedule.scss

0xdevalias

Most of the review comments here are NB and can be cleaned up in a followup PR. There are a couple of things that should be fixed up in this PR though (though they are relatively small/quick fixes)

I'm going to pre-emptively approve this to unblock you, but please make sure to fix though not-NB comments before merging (and maybe get @mike-lvov to skim through them to ensure they seem to fix the issues if you want to be certain)

src/api/profile.ts

src/components/molecules/EventDisplay/EventDisplay.tsx

src/components/molecules/EventPaymentButton/EventPaymentButton.tsx

src/components/molecules/NavBar/NavBar.tsx

src/utils/time.ts

…tion

src/components/molecules/Loading/Loading.tsx

0xdevalias

[Retroactive Review] A final pass over the changed files, calling out additional things since the last review.

@sunny-viktoryia It would be good to get these fixed up in a future followup PR, as well as the outstanding items called out in previous reviews.

src/assets/icons/icon-loading.svg

0xdevalias · 2021-05-20T09:07:15Z

src/components/molecules/Loading/Loading.tsx

+  message?: string;
+}
+
+export const Loading: FC<LoadingProps> = ({ message }) => {


React.FC instead of just FC.

Also, message should have a default value here. Probably "Loading..."

used React.FC: 880f966

and about default message. hmm. I wouldn't specify that. According to our styleguide there can be loading without text.
http://sapiens.chat/sparkle/styleguide/

So, in case the text is not needed we can use this component.

what would you say?

and about default message. hmm. I wouldn't specify that. According to our styleguide there can be loading without text.

I guess it depends what is likely to be our more common pattern. If we are going to write "Loading..." all over the place then it probably makes sense to default the text and then potentially add another optional prop for imageOnly or similar.

If we're more likely to use it without text, then not setting the default might be a better idea.

cc // @mike-lvov Thoughts?

@0xdevalias I would say that not having the default loading text makes sense just to handle a case when there is no text at all as per

According to our styleguide there can be loading without text

For other cases, I think we explicitly more specific message than Loading...

src/components/molecules/NavBar/NavBar.tsx

src/components/molecules/Schedule/Schedule.tsx

src/components/molecules/ScheduleEvent/ScheduleEvent.tsx

src/components/templates/ArtPiece/ArtPiece.tsx

src/components/molecules/ScheduleVenueDescription/ScheduleVenueDescription.tsx

src/components/organisms/NavBarSchedule/NavBarSchedule.tsx

igachmovmelon requested a review from mike-lvov April 2, 2021 12:08

igachmovmelon temporarily deployed to feature-preview April 2, 2021 12:08 Inactive

github-actions bot added the 💎 styles For (S)CSS style related issues/changes/improvements/etc. label Apr 2, 2021

github-actions bot assigned igachmovmelon Apr 2, 2021

codeclimate bot reviewed Apr 2, 2021

View reviewed changes

src/components/molecules/EventRoomDisplay/EventRoomDisplay.tsx Outdated Show resolved Hide resolved

igachmovmelon changed the title ~~Feature/schedule~~ Feature/schedule ohbm Apr 2, 2021

igachmovmelon temporarily deployed to feature-preview April 2, 2021 16:32 Inactive

igachmovmelon temporarily deployed to feature-preview April 2, 2021 16:33 Inactive

0xdevalias added the 💡 new-feature New idea/feature request label Apr 6, 2021

0xdevalias self-requested a review April 6, 2021 03:49

mike-lvov requested changes Apr 9, 2021

View reviewed changes

igachmovmelon temporarily deployed to feature-preview April 9, 2021 11:45 Inactive

0xdevalias changed the title ~~Feature/schedule ohbm~~ Improve the Event Schedule UI Apr 12, 2021

0xdevalias temporarily deployed to feature-preview April 12, 2021 03:50 Inactive

mike-lvov assigned sunny-viktoryia and unassigned igachmovmelon Apr 14, 2021

igachmovmelon and others added 9 commits April 29, 2021 15:27

add arrow to messagebox

ce1979d

Schedule for demo

05887d2

Chatbox from staging

b3636b7

sticky rooms

6b6c57b

showing time

5717560

merge

545575b

Remove unused/old schedule components

a6cd04b

Base Schedule functionality

020e7e5

Save events to the database; refactor the code

d97a828

sunny-viktoryia force-pushed the feature/schedule branch from 598ce33 to d97a828 Compare May 5, 2021 11:17

sunny-viktoryia added 3 commits May 17, 2021 23:44

Refactor getTimeBeforeParty

8b3d927

Refactor isEventWithinDate()

799902a

Remove weekday property from ScheduleDay

6d26a6f

mike-lvov approved these changes May 17, 2021

View reviewed changes

0xdevalias approved these changes May 18, 2021

View reviewed changes

0xdevalias changed the title ~~Improve the Event Schedule UI~~ refactor Event Schedule UI for new design + add personal schedule May 18, 2021

sunny-viktoryia added 7 commits May 18, 2021 11:25

Add a few comments

03d7830

Leave NavBarSchedule in NavBar only

367c7e8

Move DEFAULT_VENUE_NAME to ScheduleVenueDescription

41bfff8

Reverse isEventStartingSoon logic

ca1b2af

Add comment about NavBarSchedule class name

1bc21ee

Fix typo isPersonalizedEvent

bbb1862

Refactor Schedule related components

61c8d31

sunny-viktoryia force-pushed the feature/schedule branch from bc3ac72 to 61c8d31 Compare May 18, 2021 09:39

sunny-viktoryia added 2 commits May 18, 2021 12:53

Rename default label to ScheduleVenueDescription

eaea734

Created NavBarSchedule container to distinguish in/out clicks

a37241a

sunny-viktoryia force-pushed the feature/schedule branch from fef4bdc to a37241a Compare May 18, 2021 10:42

sunny-viktoryia added 4 commits May 18, 2021 14:10

Create NavBarScheduleClassName const

aa7ef18

Merge branch 'staging' into feature/schedule

8a48aea

Use new hooks that fetch related venues

959ba68

Add Loading component; fix the calculation for current time line posi…

df146ea

…tion

sunny-viktoryia temporarily deployed to feature-preview May 18, 2021 13:48 Inactive

mike-lvov reviewed May 18, 2021

View reviewed changes

src/components/molecules/Loading/Loading.tsx Show resolved Hide resolved

mike-lvov merged commit d5e8fa7 into staging May 18, 2021

mike-lvov deleted the feature/schedule branch May 18, 2021 14:08

mike-lvov mentioned this pull request May 18, 2021

deploy staging -> master #1415

Merged

0xdevalias reviewed May 20, 2021

View reviewed changes

sunny-viktoryia added a commit that referenced this pull request May 20, 2021

Fixes for the comments left in the source PR #1302

880f966

0xdevalias mentioned this pull request May 25, 2021

add optional scheduleStartDate to DB + force NavBarSchedule to start from that date when present #1419

Merged

sunny-viktoryia mentioned this pull request Jun 25, 2021

AAA I can set an announcement with funnels and CTA button #1440

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor Event Schedule UI for new design + add personal schedule #1302

refactor Event Schedule UI for new design + add personal schedule #1302

igachmovmelon commented Apr 2, 2021 •

edited by sunny-viktoryia

codeclimate bot commented Apr 2, 2021 •

edited

github-actions bot commented Apr 2, 2021 •

edited

0xdevalias commented Apr 7, 2021

mike-lvov commented Apr 7, 2021

mike-lvov left a comment

mike-lvov May 17, 2021

0xdevalias May 18, 2021

mike-lvov May 17, 2021

0xdevalias May 18, 2021 •

edited

mike-lvov May 18, 2021

0xdevalias May 19, 2021

mike-lvov May 17, 2021

0xdevalias May 18, 2021

mike-lvov May 18, 2021

sunny-viktoryia May 19, 2021

0xdevalias May 20, 2021

mike-lvov May 17, 2021

0xdevalias May 18, 2021

0xdevalias left a comment

0xdevalias left a comment

0xdevalias May 20, 2021

sunny-viktoryia May 20, 2021

0xdevalias May 25, 2021

mike-lvov May 27, 2021 •

edited

refactor Event Schedule UI for new design + add personal schedule #1302

refactor Event Schedule UI for new design + add personal schedule #1302

Conversation

igachmovmelon commented Apr 2, 2021 • edited by sunny-viktoryia

codeclimate bot commented Apr 2, 2021 • edited

github-actions bot commented Apr 2, 2021 • edited

0xdevalias commented Apr 7, 2021

mike-lvov commented Apr 7, 2021

mike-lvov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xdevalias May 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xdevalias left a comment

Choose a reason for hiding this comment

0xdevalias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-lvov May 27, 2021 • edited

Choose a reason for hiding this comment

igachmovmelon commented Apr 2, 2021 •

edited by sunny-viktoryia

codeclimate bot commented Apr 2, 2021 •

edited

github-actions bot commented Apr 2, 2021 •

edited

0xdevalias May 18, 2021 •

edited

mike-lvov May 27, 2021 •

edited