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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make order details panel visible. #2719

Merged
merged 21 commits into from
Sep 12, 2017

Conversation

foladipo
Copy link
Contributor

@foladipo foladipo commented Aug 22, 2017

Resolves #2617.

This PR resolves a bug with the orders page. Using the Collapsed mode to view the list of orders, the details panel for any specific order wasn't showing up because of some CSS issues.

Test instructions

  • Log in as admin and make one or two orders.
  • Use the admin menu (on the right) to go to the Orders page.
  • Switch to Collapsed view.
  • Click the > beside any listed order and you'll notice that the details panel for that order now show up correctly in the browser.
  • Try to resize the browser window. You'll notice that the order details panel stays visible throughout.

@brent-hoover
Copy link
Collaborator

When I first open the orders panel I get this big blank space where there should be none:

thfh7gymvdgsrzdms

@brent-hoover
Copy link
Collaborator

It seems like at wider widths we would want the main order grid to fill all available space and possibly add more fields rather than just having a blank space there.

thfh7gymvdgsrzdms

@brent-hoover
Copy link
Collaborator

At more narrow widths we don't want to lose the order selector there.

thfh7gymvdgsrzdms

@rymorgan Is there an already existing spec for how this order table should operate at all the varying widths? I feel like there is but I am not sure where one would look.

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

Still not behaving correctly at all widths

@foladipo
Copy link
Contributor Author

@zenweasel Thanks for the feedback. I have tested this PR and it truly displays the orders table in a weird way at wider screen widths. I'll fix that and update my PR.

As for the orders' table's behaviour at smaller widths, especially in the screenshot you shared, what happens is that the container for the table becomes scrollable. So, if you scroll to the right, the order selector is still visible.

PS: Other than the above, this PR also hides various columns as the screen size reduces in order to fit the orders' table to the screen.

@aaronjudd aaronjudd assigned foladipo and unassigned brent-hoover Aug 25, 2017
@brent-hoover brent-hoover changed the title Make order details panel visible. [WIP] Make order details panel visible. Aug 25, 2017
@aaronjudd
Copy link
Contributor

@foladipo can you come back to this soon, and fix the merge conflicts, and get this ready for review, or can you close the PR until you are ready?

@foladipo
Copy link
Contributor Author

Thanks for the heads-up @aaronjudd . I put this in the icebox to work on #2265 because of how important it was to fix that bug.

I've now fixed the issues observed by @zenweasel and pushed the commits.

@foladipo foladipo changed the title [WIP] Make order details panel visible. Make order details panel visible. Aug 30, 2017
@brent-hoover
Copy link
Collaborator

This is what I am seeing upon opening the order dashboard for the first time

dashboard_orders

@brent-hoover
Copy link
Collaborator

@foladipo I thought the above corrupted view might be dependent on a particular width but that's the way it is at all widths, rendering the order panel unusable.

@foladipo
Copy link
Contributor Author

@zenweasel I've tried to reproduce this both on my computer's screen and a large monitor but can't. Can you tell me the steps you carried out to get the bug?

I also wonder why the order's detail panel (to the right) is actually duplicating itself.

@brent-hoover
Copy link
Collaborator

@foladipo I followed the instructions in the PR

@rymorgan
Copy link
Contributor

rymorgan commented Aug 31, 2017

screen shot 2017-08-31 at 11 45 37 am

I'm seeing the same thing. There is something more going on with sidebar than just this. I created an issue for what I'm seeing in general with the sidebar UI in orders. The sidebar for orders is basically unusable in Marketplace right now.

#2775

@foladipo
Copy link
Contributor Author

foladipo commented Sep 5, 2017

@rymorgan @zenweasel So, I've figured out where this happens, being guided by what Ryan reported in #2775. Apparently, you can reproduce this bug as follows:

  • Add items to your cart and checkout (i.e pay for the products and get to the "Thank you" page.

  • While on the checkout page, try to open the orders panel. It will open as a modal, as expected

  • Click the blue, "manage order" button beside any new order. Instead of still have a modal, the orders panel is now a main page by itself, and the order details panel at the side is duplicated. And instead of the URL staying the same, it's changed to http://localhost:XXXX/dashboard/orders?_id=XXXXXXXXXX. Apparently, the "manage order" button now links to /dashboard/orders, instead of opening a side panel.

  • If you go to the products' dashboard/grid and try to open the orders panel again, it functions as normal.

I'm guessing this was caused by some new code pushed to marketplace. I'll investigate it further, try to fix it and add that fix to this one.

(Also add a TODO about a needed discussion of code design.)
@foladipo
Copy link
Contributor Author

foladipo commented Sep 5, 2017

@rymorgan and @zenweasel So, I've figured out that it doesn't even happen on the "Thank you" page alone. It happens on all pages that are NOT the index. So, if you go to localhost:XXXX/product/XXXX or some other non-index URL, you will see the orders panel misbehave.

I eventually traced the source down to this section of code. So, I wish to know your corrections (e.g that that code isn't actually the root of the bug) or suggestions as to how to update that section to fix the bug WHILE not breaking other parts of the codebase.

@rymorgan
Copy link
Contributor

rymorgan commented Sep 5, 2017

@mikemurray @kieckhafer Can one of you two take a look at this. I can get you up to speed on the bug if needed.

@mikemurray
Copy link
Member

@foladipo The action view is split up into 2 sections.

[ actionView | actionViewDetail ]

You open the main action view with Reaction.setActionView() and the detail view Reaction.setActionViewDetail(), which opens a smaller sidepanel inside the big action view panel.

The button that opens the order detail should be doing Reaction.setActionViewDetail().

@foladipo
Copy link
Contributor Author

foladipo commented Sep 6, 2017

@mikemurray Yes, the button that opens the order detail does call Reaction.setActionViewDetail().

The problem is that that call eventually gets into this function. What I don't understand is why we have to open either admin/detailView or admin/actionView based on whether or not we are on the index page.

In fact, if I simply open admin/detailView always, it fixes this new bug reported by Brent and Ryan.

Now, after doing some more digging, I found out that that section of code was written by Jeremy about two weeks ago in this commit.

@jshimko Please can you explain the aim of that section of code?

@brent-hoover
Copy link
Collaborator

@foladipo I don't know that Jeremy will know any thing particularly about that code as it was a merge commit. It was probably to fix a bug but that history has probably been lost with development.

Either way, does this current code fix this bug? I am going to re-test now because it's my assumption from your notes that that's the status and it's not marked [WIP].

@brent-hoover
Copy link
Collaborator

So it looks like this is still broken.

@mikemurray Can you help us understand why/when/if we would want to open admin/adminView for orders if there is any case for that? Is there no case for this (even for index right?). Maybe this is just some merge weirdness and we can get rid of that if branch.

@mikemurray
Copy link
Member

@zenweasel @foladipo I'm not sure why there is a check for the index page in the actionView function. It shouldn't care about that at all.

I think at some point that index was meant for the internal navigation stack of the action view itself, and not the actual Reaction Router routing/pages.

@brent-hoover
Copy link
Collaborator

So I just removed that branching since no one could understand why it's there and everything about orders seems to work great. A quick check of admin panels seems to be fine as well.

@mikemurray Would you mind taking one last look at this and make sure everything is operating as it should be?

Copy link
Member

@mikemurray mikemurray left a comment

Choose a reason for hiding this comment

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

When I click the reaction logo, I think the actionViewDetail is being opened on the small view of the actionView panel. This doesn't seem to happen with the other button in the icon toolbar. Might be simple enough to close the detail view when you click on the reaction button. Otherwise it appears to be fixed.

To reproduce:

  1. Click on the orders Icon in the icon toolbar
  2. Open an order (show the small side panel)
  3. Click the reaction logo

cart_completed

cart_completed_and_slack_-_reaction_and_calendar

@foladipo
Copy link
Contributor Author

@mikemurray Thanks for the heads up. That bug has now been fixed.

cc @zenweasel

Copy link
Member

@mikemurray mikemurray left a comment

Choose a reason for hiding this comment

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

@foladipo

Found another issue:

cart_completed

cart_completed

cart_completed

cc @zenweasel

@mikemurray mikemurray dismissed their stale review September 12, 2017 17:30

Was on marketplace branch when testing. My bad. All your changes are good!

@mikemurray mikemurray merged commit 0c46580 into marketplace Sep 12, 2017
@mikemurray mikemurray deleted the folusho-make-order-detail-visible-2617 branch September 12, 2017 17:30
@spencern spencern mentioned this pull request Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants