TASK-12: discovery page tabs #92

r-saba · 2020-11-27T01:01:49Z

This PR implements the discovery page tabs #54.

To test the component visit the followin URL:
http://localhost:4200/home

Swiping between routes is possible when in mobile view

cristian-aldea

Nice implementation @r-saba! I had a few concerns and comments I'd like some clarifications on, but otherwise nice job.

package.json

apps/mull-ui/src/app/app.scss

apps/mull-ui/src/app/app.tsx

apps/mull-ui/src/app/components/subnavigation-bar/subnavigation-bar.tsx

apps/mull-ui/src/app/app.tsx

cristian-aldea

Thanks for addressing my comments, I don't have time to re-review, and I don't want to block this pr, so I'll approve it immediately if it gets another approval. I'll be re-reviewing in more detail tomorrow.

bit172

Looks very good! I just have a really minor change. Good job!

bit172 · 2020-11-28T03:29:58Z

apps/mull-ui/src/constants.ts

  MAP: '/map',
  CREATE_EVENT: '/create-event',
  TOOLS: '/tools',
  MESSAGES: '/messages',
  PROFILE: '/profile',
  LOGIN: '/login',
  REGISTER: '/register',
+  DISCOVER: '/discover',
+  UPCOMING: '/upcoming',
+  MYEVENTS: '/myevents',


This is really minor but I think it should be consistent

Suggested change

MYEVENTS: '/myevents',

MY_EVENTS: '/my-events',

JimLau49

Looks good to me 🚀 . I think it should have been mentioned that to test scrollability that you need to click along the length of container that has "DISCOVER!" "UPCOMING!" or "MYEVENTS!"
Acceptance Criteria

The tabs are implemented for the discovery page and the user can swipe/tap to switch between tabs.
Each tab is routed, but can lead to dead links or pages
Documentation present for every new component and function
Unit and UI tests are implemented for every component and function
Must match Mockup

cristian-aldea

I had a chance to look at your pr in more detail and I found a few more issues I'd like addressed or replied to.

cristian-aldea · 2020-11-28T15:45:08Z

apps/mull-ui/src/app/app.tsx

@@ -41,6 +44,15 @@ export const App = () => {
            <EventPage event={dummyEvent} prevPage={'Review'} />
          </div>
        </Route>
+        {/* Without this outter Route component, the inner components would show on all routes that are not matched */}
+        <Route exact path={[`${ROUTES.DISCOVER}`, `${ROUTES.UPCOMING}`, `${ROUTES.MYEVENTS}`]}>


Is there a reason why we can't have:

Suggested change

<Route exact path={[`${ROUTES.DISCOVER}`, `${ROUTES.UPCOMING}`, `${ROUTES.MYEVENTS}`]}>

<Route exact path={[ROUTES.DISCOVER, ROUTES.UPCOMING, ROUTES.MYEVENTS]}>

I tried it and it seems to work

cristian-aldea · 2020-11-28T15:46:44Z

apps/mull-ui/src/app/app.tsx

+        <Route exact path={[`${ROUTES.DISCOVER}`, `${ROUTES.UPCOMING}`, `${ROUTES.MYEVENTS}`]}>
+          <SubNavigationBar />
+          <SwipeableRoutes>
+            <Route path={ROUTES.DISCOVER} component={() => <div>DISCOVER!</div>} />
+            <Route path={ROUTES.UPCOMING} component={() => <div>UPCOMING!</div>} />
+            <Route path={ROUTES.MYEVENTS} component={() => <div>MYEVENTS!</div>} />
+          </SwipeableRoutes>
+        </Route>


One issue im seeing right now with styling is that the Subnavbar is set to be in the page-container, which is then pushing the actual page content further down than it should.

I propose this solution: Remove the page-container class from SubNavigationBar, and put page-container on a container div. So something like this:

Suggested change

<Route exact path={[`${ROUTES.DISCOVER}`, `${ROUTES.UPCOMING}`, `${ROUTES.MYEVENTS}`]}>

<SubNavigationBar />

<SwipeableRoutes>

<Route path={ROUTES.DISCOVER} component={() => <div>DISCOVER!</div>} />

<Route path={ROUTES.UPCOMING} component={() => <div>UPCOMING!</div>} />

<Route path={ROUTES.MYEVENTS} component={() => <div>MYEVENTS!</div>} />

</SwipeableRoutes>

</Route>

<Route exact path={[ROUTES.DISCOVER, ROUTES.UPCOMING, ROUTES.MYEVENTS]}>

<div className="page-container">

<SubNavigationBar />

<SwipeableRoutes>

<Route path={ROUTES.DISCOVER} component={() => <div>DISCOVER!</div>} />

<Route path={ROUTES.UPCOMING} component={() => <div>UPCOMING!</div>} />

<Route path={ROUTES.MYEVENTS} component={() => <div>MYEVENTS!</div>} />

</SwipeableRoutes>

</div>

</Route>

cristian-aldea · 2020-11-28T15:48:13Z

apps/mull-ui/src/app/app.tsx

@@ -41,6 +44,15 @@ export const App = () => {
            <EventPage event={dummyEvent} prevPage={'Review'} />
          </div>
        </Route>
+        {/* Without this outter Route component, the inner components would show on all routes that are not matched */}
+        <Route exact path={[`${ROUTES.DISCOVER}`, `${ROUTES.UPCOMING}`, `${ROUTES.MYEVENTS}`]}>


The more I think about it the more I think these routes should be under /home, since each tab is a subpage for the home page. I'm not gonna block the PR over this, but I really think what you did initially was good.

Sure, I will change it

apps/mull-ui/src/app/components/subnavigation-bar/subnavigation-bar.tsx

cristian-aldea · 2020-11-28T15:56:56Z

apps/mull-ui/src/app/components/subnavigation-bar/subnavigation-bar.spec.tsx

+      [`${ROUTES.UPCOMING}`]: 'subnavigation-upcoming-button',
+      [`${ROUTES.MYEVENTS}`]: 'subnavigation-myEvents-button',


Same comment as in app.tsx, is there a reason we can't put:

Suggested change

[`${ROUTES.UPCOMING}`]: 'subnavigation-upcoming-button',

[`${ROUTES.MYEVENTS}`]: 'subnavigation-myEvents-button',

[ROUTES.UPCOMING]: 'subnavigation-upcoming-button',

[ROUTES.MYEVENTS]: 'subnavigation-myEvents-button',

I tried it and the tests still seem to work.

It's a remnant from my first iteration, I did a simple find and replace, should be what you proposed now.

r-saba · 2020-11-28T21:20:21Z

I addressed your pr comments @bit172 @TheGreatMarkus @JimLau49 . Let me know how it's looking.
Major difference is now each tab points to a nested route /home/:id

cristian-aldea

Great Work Ragith! I just had one last issue I saw. But otherwise this is as good as merged.

The tabs are implemented for the discovery page and the user can swipe/tap to switch between tabs.
Each tab is routed but can lead to dead links or pages
Documentation present where needed
Unit and UI tests are implemented for every component and function
Must match Mockup

apps/mull-ui/src/app/app.tsx

bit172

LGTM! 🚀

The tabs are implemented for the discovery page and the user can swipe/tap to switch between tabs.
Each tab is routed but can lead to dead links or pages
Documentation present where needed
Unit and UI tests are implemented for every component and function
Must match Mockup

… page shown #54

r-saba force-pushed the TASK-12/discovery-page-tabs branch from ce1c7ec to 2bc04f6 Compare November 27, 2020 01:05

r-saba requested review from RGPosadas, cristian-aldea, bit172, cindyslittleplanet, JimLau49, leojrsilao, osahtout and ys-lin November 27, 2020 01:06

r-saba added adds npm packages PR introduces new npm dependencies to the project frontend labels Nov 27, 2020

cristian-aldea changed the title ~~Task 12/discovery page tabs~~ TASK-12: discovery page tabs Nov 27, 2020

cristian-aldea requested changes Nov 28, 2020

View reviewed changes

cristian-aldea reviewed Nov 28, 2020

View reviewed changes

bit172 requested changes Nov 28, 2020

View reviewed changes

JimLau49 previously approved these changes Nov 28, 2020

View reviewed changes

cristian-aldea requested changes Nov 28, 2020

View reviewed changes

r-saba dismissed JimLau49’s stale review via 223355c November 28, 2020 21:11

r-saba force-pushed the TASK-12/discovery-page-tabs branch from 223355c to 82d35d4 Compare November 28, 2020 21:16

r-saba requested review from cristian-aldea, bit172 and JimLau49 November 28, 2020 21:20

cristian-aldea previously approved these changes Nov 28, 2020

View reviewed changes

apps/mull-ui/src/app/app.tsx Show resolved Hide resolved

JimLau49 previously approved these changes Nov 28, 2020

View reviewed changes

bit172 previously approved these changes Nov 28, 2020

View reviewed changes

r-saba added 4 commits November 28, 2020 17:46

Add subnavigation menu without swipe functionality #54

5e1352b

Add swipeable routes, failing tests #54

c35a074

Remove snapshot tests for app.tsx and change routing logic #54

9b87624

Add tests for subnavigation active buttons #54

df6efb4

r-saba added 5 commits November 28, 2020 17:46

Add specific routes for the subnavigation, make /discover the initial…

8ec9cbd

… page shown #54

refactor: Remove unecessary variables from existing tests #54

a50302f

Address pr comments #54

dc39ecc

Address more pr comments #54

e02beba

Add redirect for all /home/* routes #54

5ed3192

r-saba dismissed stale reviews from bit172, JimLau49, and cristian-aldea via 5ed3192 November 28, 2020 22:52

r-saba force-pushed the TASK-12/discovery-page-tabs branch from 82d35d4 to 5ed3192 Compare November 28, 2020 22:52

r-saba requested review from cristian-aldea, JimLau49 and bit172 November 28, 2020 22:53

JimLau49 previously approved these changes Nov 28, 2020

View reviewed changes

bit172 previously approved these changes Nov 28, 2020

View reviewed changes

cristian-aldea previously approved these changes Nov 28, 2020

View reviewed changes

Fix prettier issue with pr #54

ea8b537

r-saba dismissed stale reviews from cristian-aldea, bit172, and JimLau49 via ea8b537 November 28, 2020 23:29

JimLau49 approved these changes Nov 28, 2020

View reviewed changes

cristian-aldea approved these changes Nov 28, 2020

View reviewed changes

r-saba merged commit eaa3299 into develop Nov 28, 2020

cristian-aldea added the refactor label Nov 29, 2020

r-saba deleted the TASK-12/discovery-page-tabs branch December 20, 2020 02:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TASK-12: discovery page tabs #92

TASK-12: discovery page tabs #92

r-saba commented Nov 27, 2020 •

edited

cristian-aldea left a comment

cristian-aldea left a comment •

edited

bit172 left a comment

bit172 Nov 28, 2020

JimLau49 left a comment •

edited

cristian-aldea left a comment

cristian-aldea Nov 28, 2020

cristian-aldea Nov 28, 2020

cristian-aldea Nov 28, 2020

r-saba Nov 28, 2020

cristian-aldea Nov 28, 2020

r-saba Nov 28, 2020 •

edited

r-saba commented Nov 28, 2020

cristian-aldea left a comment

bit172 left a comment

	<Route exact path={[`${ROUTES.DISCOVER}`, `${ROUTES.UPCOMING}`, `${ROUTES.MYEVENTS}`]}>
	<Route exact path={[ROUTES.DISCOVER, ROUTES.UPCOMING, ROUTES.MYEVENTS]}>

		[`${ROUTES.UPCOMING}`]: 'subnavigation-upcoming-button',
		[`${ROUTES.MYEVENTS}`]: 'subnavigation-myEvents-button',

TASK-12: discovery page tabs #92

TASK-12: discovery page tabs #92

Conversation

r-saba commented Nov 27, 2020 • edited

cristian-aldea left a comment

Choose a reason for hiding this comment

cristian-aldea left a comment • edited

Choose a reason for hiding this comment

bit172 left a comment

Choose a reason for hiding this comment

bit172 Nov 28, 2020

Choose a reason for hiding this comment

JimLau49 left a comment • edited

Choose a reason for hiding this comment

cristian-aldea left a comment

Choose a reason for hiding this comment

cristian-aldea Nov 28, 2020

Choose a reason for hiding this comment

cristian-aldea Nov 28, 2020

Choose a reason for hiding this comment

cristian-aldea Nov 28, 2020

Choose a reason for hiding this comment

r-saba Nov 28, 2020

Choose a reason for hiding this comment

cristian-aldea Nov 28, 2020

Choose a reason for hiding this comment

r-saba Nov 28, 2020 • edited

Choose a reason for hiding this comment

r-saba commented Nov 28, 2020

cristian-aldea left a comment

Choose a reason for hiding this comment

bit172 left a comment

Choose a reason for hiding this comment

r-saba commented Nov 27, 2020 •

edited

cristian-aldea left a comment •

edited

JimLau49 left a comment •

edited

r-saba Nov 28, 2020 •

edited