Saleor 1712 dashboard unable to handle empty channel list #924

MarekChoinski · 2020-12-28T16:36:48Z

I want to merge this change because... it handles empty channels on:

homepage
/dashboard/channels
/dashboard/orders
/dashboard/products

NOTE: Tested by mocking up response to [{"data": {"channels": []}}] on AppChannelContext.tsx.

PR intended to be tested with API branch:

Screenshots

Pull Request Checklist

All visible strings are translated with proper context.
All data-formatting is locale-aware (dates, numbers, and so on).
Translated strings are extracted.
Number of API calls is optimized.
The changes are tested.
Data-test are added for new elements.
Type definitions are up to date.
Changes are mentioned in the changelog.

Test environment config

API_URI=https://master.staging.saleor.rocks/graphql/

src/categories/views/CategoryDetails.tsx

bmigirl · 2021-01-04T08:49:23Z

src/categories/views/CategoryDetails.tsx

+          </ActionDialog>
+        </>
+      ) : (
+        <Skeleton />


Maybe for easier readability, instead of making a huge positive condition outcome and then simple skeleton, put the short and simple outcome before everything else?

if(!channel) { return <><TitleSomething><Skeleton /></> } return everything as normal

bmigirl · 2021-01-04T09:10:52Z

src/home/views/index.tsx


 const HomeSection = () => {
  const navigate = useNavigator();
  const { user } = useUser();
  const { channel } = useAppChannel();

+  const noChannel = !channel && typeof channel !== "undefined";


I'd rather see this as a positive const, such as channelExists or hasChannel, especially since most places we use it, it's negated. Besides that it seems easier to overall think of positives +- negation, rather than negatives +- negation (that more easily created negated negative), don't you think?

I left noChannel after the refactor in which I added default props in HomePage. In this component it's mostly positive value. What do you think?

bmigirl · 2021-01-04T09:11:42Z

src/home/views/index.tsx

-    </HomePageQuery>
+    <HomePage
+      activities={data?.activities.edges.map(edge => edge.node).reverse()}
+      orders={!noChannel ? data?.ordersToday.totalCount : 0}


I think in this case HomePage component should have a default when orders is undefined. Then it also doesn't require to check if !noChannel

bmigirl · 2021-01-04T09:11:54Z

src/home/views/index.tsx

+      orders={!noChannel ? data?.ordersToday.totalCount : 0}
+      sales={maybe(() => data?.salesToday.gross)}
+      topProducts={
+        !noChannel ? data?.productTopToday.edges.map(edge => edge.node) : null


Same as above

bmigirl · 2021-01-04T09:12:41Z

src/home/views/index.tsx

+          })
+        )
+      }
+      ordersToCapture={!noChannel ? data?.ordersToCapture.totalCount : 0}


63-65 same as above

bmigirl · 2021-01-04T09:12:53Z

src/home/views/index.tsx

+      ordersToFulfill={!noChannel ? data?.ordersToFulfill.totalCount : 0}
+      productsOutOfStock={!noChannel ? data?.productsOutOfStock.totalCount : 0}
+      userName={getUserName(user, true)}
+      userPermissions={user?.userPermissions || []}


Also should be default in home page

bmigirl · 2021-01-04T09:13:34Z

src/index.tsx

@@ -145,7 +145,7 @@ const Routes: React.FC = () => {
  return (
    <>
      <WindowTitle title={intl.formatMessage(commonMessages.dashboard)} />
-      {channel &&
+      {typeof channel !== "undefined" &&


Would happily see those 4 &&'s moved to a const with a nice name

bmigirl · 2021-01-04T09:14:04Z

src/orders/views/OrderList/OrderList.tsx

@@ -71,6 +71,8 @@ export const OrderList: React.FC<OrderListProps> = ({ params }) => {

  const { channel, availableChannels } = useAppChannel();

+  const noChannel = !channel && typeof channel !== "undefined";


Same as before

bmigirl · 2021-01-04T09:18:44Z

src/home/components/HomePage/HomePage.tsx

@@ -99,7 +103,9 @@ const HomePage: React.FC<HomePageProps> = props => {
                  />
                }
              >
-                {sales ? (
+                {noChannel ? (


Maybe if there are three outcomes here, it'd be worth to just move it to a function? Chaining conditions makes it less readable. Also, would rather see this in positive, like channelExists + negation

orzechdev · 2021-01-04T09:19:51Z

src/home/views/index.tsx

+  const noChannel = !channel && typeof channel !== "undefined";
+
+  const { data } = !noChannel
+    ? useHomePage({


I'm afraid conditionally using hook might cause React errors in the future - https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level

src/home/views/index.tsx

jwm0

I clicked through the dashboard and there's a lot of pages that do not work because they "cannot read property id of undefined". Please check where we use channel.id and replace it with channel?.id. Products/categories/orders/discounts should be accessible

MarekChoinski requested review from krzysztofwolski, dominik-zeglen, orzechdev and bmigirl December 28, 2020 16:36

github-actions bot temporarily deployed to saleor-1712-dashboard-unable-to-handle-empty-channel-list December 28, 2020 16:37 Inactive

github-actions bot temporarily deployed to storybook saleor-1712-dashboard-unable-to-handle-empty-channel-list December 28, 2020 16:37 Inactive

MarekChoinski marked this pull request as ready for review December 28, 2020 17:43

MarekChoinski force-pushed the SALEOR-1712-dashboard-unable-to-handle-empty-channel-list branch from c2ff1f5 to 1bd4385 Compare December 28, 2020 18:04

github-actions bot temporarily deployed to saleor-1712-dashboard-unable-to-handle-empty-channel-list December 28, 2020 18:04 Inactive

github-actions bot temporarily deployed to storybook saleor-1712-dashboard-unable-to-handle-empty-channel-list December 28, 2020 18:04 Inactive

bmigirl suggested changes Jan 4, 2021

View reviewed changes

orzechdev suggested changes Jan 4, 2021

View reviewed changes

MarekChoinski added 9 commits January 5, 2021 16:21

Use query hook on home page

5edd343

Handle no channel

2a67688

Handle no channel on pages

9417345

Add navigate channels

1d03e07

Move messages

8041c7e

Refactor

5d66ce8

Update storybook and locale

4ee840b

Remove comment

5a5bfa7

Refactor

eed545a

MarekChoinski force-pushed the SALEOR-1712-dashboard-unable-to-handle-empty-channel-list branch from 1bd4385 to eed545a Compare January 7, 2021 09:30

github-actions bot temporarily deployed to saleor-1712-dashboard-unable-to-handle-empty-channel-list January 7, 2021 09:31 Inactive

github-actions bot temporarily deployed to storybook saleor-1712-dashboard-unable-to-handle-empty-channel-list January 7, 2021 09:31 Inactive

Update storybook

f14f1ad

github-actions bot temporarily deployed to saleor-1712-dashboard-unable-to-handle-empty-channel-list January 7, 2021 09:54 Inactive

github-actions bot temporarily deployed to storybook saleor-1712-dashboard-unable-to-handle-empty-channel-list January 7, 2021 09:54 Inactive

MarekChoinski requested review from orzechdev and bmigirl January 7, 2021 10:07

orzechdev reviewed Jan 7, 2021

View reviewed changes

src/home/views/index.tsx Outdated Show resolved Hide resolved

Fix skip

1ce5207

github-actions bot temporarily deployed to saleor-1712-dashboard-unable-to-handle-empty-channel-list January 7, 2021 21:54 Inactive

github-actions bot temporarily deployed to storybook saleor-1712-dashboard-unable-to-handle-empty-channel-list January 7, 2021 21:54 Inactive

bmigirl approved these changes Jan 8, 2021

View reviewed changes

jwm0 requested review from orzechdev and removed request for krzysztofwolski and dominik-zeglen January 8, 2021 16:03

orzechdev approved these changes Jan 11, 2021

View reviewed changes

jwm0 suggested changes Jan 12, 2021

View reviewed changes

Fix undefined channel

7e14dea

github-actions bot temporarily deployed to saleor-1712-dashboard-unable-to-handle-empty-channel-list January 12, 2021 14:27 Inactive

github-actions bot temporarily deployed to storybook saleor-1712-dashboard-unable-to-handle-empty-channel-list January 12, 2021 14:27 Inactive

Update storybook

23cad88

github-actions bot temporarily deployed to saleor-1712-dashboard-unable-to-handle-empty-channel-list January 13, 2021 09:09 Inactive

github-actions bot deployed to storybook saleor-1712-dashboard-unable-to-handle-empty-channel-list January 13, 2021 09:09 View deployment

jwm0 approved these changes Jan 13, 2021

View reviewed changes

jwm0 merged commit d0be941 into master Jan 13, 2021

jwm0 deleted the SALEOR-1712-dashboard-unable-to-handle-empty-channel-list branch January 13, 2021 09:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Saleor 1712 dashboard unable to handle empty channel list #924

Saleor 1712 dashboard unable to handle empty channel list #924

MarekChoinski commented Dec 28, 2020

bmigirl Jan 4, 2021

bmigirl Jan 4, 2021

MarekChoinski Jan 7, 2021

bmigirl Jan 4, 2021

bmigirl Jan 4, 2021

bmigirl Jan 4, 2021

bmigirl Jan 4, 2021

bmigirl Jan 4, 2021

bmigirl Jan 4, 2021

bmigirl Jan 4, 2021

orzechdev Jan 4, 2021

jwm0 left a comment

		@@ -71,6 +71,8 @@ export const OrderList: React.FC<OrderListProps> = ({ params }) => {

		const { channel, availableChannels } = useAppChannel();

		const noChannel = !channel && typeof channel !== "undefined";

Saleor 1712 dashboard unable to handle empty channel list #924

Saleor 1712 dashboard unable to handle empty channel list #924

Conversation

MarekChoinski commented Dec 28, 2020

Screenshots

Pull Request Checklist

Test environment config

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

Choose a reason for hiding this comment

jwm0 left a comment

Choose a reason for hiding this comment