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

Add global shop selector for multi-shop usage #297

Merged
merged 46 commits into from
Sep 22, 2020

Conversation

loan-laux
Copy link
Contributor

@loan-laux loan-laux commented Jul 29, 2020

Resolves #93
Impact: major
Type: feature

Issue

The API supports administering multiple shops, but reaction-admin doesn't provide any way to do it from the interface.

As agreed with @aldeed on issue #93, this PR is here to "test the waters" when it comes to adding shop selectors across the admin dashboard. This would replace the current mechanism of fetching the primary shop ID, in all the places where shop IDs are required to be passed to queries and mutations.

I thought I'd start with the products table as that's the number one most important place for a shop selector to be added. When we reach something that satisfies everyone, I'll add these shop selectors to other pages where they make sense (orders, tags etc).

Solution

Add a shop selector showing the shops from the user's adminUIShops (see api-plugin-accounts#17). When shops are selected, use their IDs in the relevant queries and mutations instead of the primary shop's ID.

Right now, my implementation is very basic and is limited to just showing products from different shops. There's no support of adding products to different shops. Created products are still added to the primary shop for now. This is coming, hence the draft status. I wanted nonetheless to have this PR up for discussion as early as possible, so there you have it.

When it comes to the visual aspect, I didn't really put any efforts into that. This is what I currently have, but feel free to please provide guidance as to how this should look.

Screen Shot 2020-07-29 at 17 24 49

Breaking changes

None.

Testing

  1. Go to the Products page.
  2. If your account has only one adminUIShops item, this one shop should be selected by default in the dropdown and the products for this shop should show up.
  3. If your account has multiple adminUIShops, you should have the ability to pick one or multiple of these shops to show products from.

Signed-off-by: Loan Laux <loan@outgrow.io>
@janus-reith
Copy link
Contributor

I added my considerations regarding the per-table shop select to #93 .
In general I think these table pages, including ProductTable contain too much logic already and should be abstracted further instead of putting more logic per table (DataTable is a good thing already, but I still think the way it is used in the admin still is too verbose and repetitive)

@loan-laux
Copy link
Contributor Author

I've just pushed some more commits to make users pick the shop they want to create a product for, after they click on "Create product." Of course, this modal doesn't show up if the user has admin access to just one shop. In this case, the product will be created for this one shop, the same way that it currently works on trunk.

Core team, let me know if you're happy with something like this, or if you'd rather do it in a different way.

Screen Shot 2020-07-31 at 14 19 56
Screen Shot 2020-07-31 at 14 19 51

@aldeed
Copy link
Contributor

aldeed commented Aug 5, 2020

@loan-laux Although I'm no longer core team, a modal seems like a fine solution. Adding the shop field at the top of the create form makes even more sense to me, but if memory serves, we might be creating the product behind the scenes before ever showing the create form? I don't remember if there are good reasons why we do that other than the historical needs of the old WYSIWYG product editor. As a general rule, I prefer filling out create form before doing the create GQL call.

rosshadden and others added 16 commits August 24, 2020 16:09
Signed-off-by: Ross Hadden <rosshadden@gmail.com>
Signed-off-by: Manuel Del Real <manny.delreal@mailchimp.com>
Signed-off-by: Ross Hadden <rosshadden@gmail.com>
Signed-off-by: Matias Zuniga <matias.nicolas.zc@gmail.com>
…param to all routes

Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Michał Staśkiewicz <staskiewicz.miko@gmail.com>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Sam Kelleher <sam@samkelleher.com>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
Signed-off-by: Loan Laux <loan@outgrow.io>
@loan-laux
Copy link
Contributor Author

Following our discussions and pending design input from @rymorgan, I've gone ahead and built a working prototype of what a global shop selector could look like.

This is what I have right now:

Screen Shot 2020-09-01 at 17 56 23
Screen Shot 2020-09-01 at 17 56 28

@mikemurray suggested that I introduce a :shopId prefix argument to all the operator URLs, which I did. This required quite a few changes to the way routes are registered, as well as the rendering of the sidebar links. Things on that side might not exactly be the cleanest for now, but honestly I just wanted to get something working quickly in order to demo the concept and validate it first.

For now, it seems to work pretty much everywhere. The only known issue on my end is the media uploader not working, but I'm fixing that as we speak.

Would really appreciate any feedback, first from the UX point of view but also on the technical implementation itself.

@loan-laux loan-laux marked this pull request as ready for review September 2, 2020 08:09
@loan-laux
Copy link
Contributor Author

@willopez & @chrispotter, let me know if you have any feedback on this. I know it's a big one and it's not necessarily a top priority, but I'd love to know at least if I'm going in the right direction with it.

@chrispotter
Copy link

@loan-laux I think this is headed in the right direction. The dropdown is in a place I'd expect it now. I'm also curious to hear @rymorgan's or @nnnnat's input as well!

@nnnnat
Copy link
Contributor

nnnnat commented Sep 8, 2020

@loan-laux at a glance the only thing I'd like to see is the drop down arrow icon have a little more contrast with the background (maybe make it the same color as the text or a similar gray/grey). I won't get a chance to pull this down and test for a few days but that shouldn't be a blocker if the rest of the team is feeling confident in this approach. I'm comfortable with less than ideal UX for the Reaction Admin in the sort term because once we've re-staffed the team to do active development on Reaction a major overhaul of developer & user experience of the Admin is something we plan on tackling within the next 6-12 months.

@loan-laux
Copy link
Contributor Author

Thanks for your feedback guys.

@nnnnat, fully agree with you on potentially breaking things on reaction-admin since it'll be replaced soon and it's currently labeled as beta. Hopefully this PR can serve as a proof of concept to re-implement multi-shop in a better way afterwards. It still needs to be clean, of course, but maybe we can live with some limitations knowing that it won't be used for long.

Let me know when you both get a chance to pull and try it! I'm really eager to get feedback on this. Also, will change the color of the arrow on the dropdown as requested.

@wittycodes
Copy link

Thanks @loan-laux for this exclusive PR, it certainly would help us alot at Craflo for time being. We created multiple clones of reaction-admin (torned & mold various major parts of UI) for different set of roles such as category manager, vendor/seller (shop manager), affiliate marketers, accounts support team. We were figuring out a temporary but a quick way for manager's reaction-admins to get easy on UI for controlling multiple shops which comes under that manager.

@loan-laux
Copy link
Contributor Author

Glad this helps, @wittycodes!

Signed-off-by: Loan Laux <loan@outgrow.io>
@loan-laux
Copy link
Contributor Author

@nnnnat I've just made the dropdown arrow the same color as the text. Let me know if you get a chance to try out the PR!

@loan-laux
Copy link
Contributor Author

@kieckhafer pinging you on here as requested on Slack. Let me know what you think!

@kieckhafer
Copy link
Member

kieckhafer commented Sep 17, 2020

Hey @loan-laux, I'm not seeing any shop dropdown on the product grid page. I have two shops, and both shopIds are in my adminUIShops array.

I do have two products, one for each store, and I'm only seeing the product for the first / default store, so a filter is working on the table, I just don't see a way to choose the store.

Is there something I'm missing, do I need to enable something?

image

@loan-laux
Copy link
Contributor Author

Hey @kieckhafer, as I said in this PR's discussion and following everyone's feedback on the first implementation, I dropped the idea of a per-table selector and implemented a global shop selector at the top-left corner of the UI. You should see an arrow point down next to your shop name. Can you see it?

@kieckhafer
Copy link
Member

kieckhafer commented Sep 17, 2020

@loan-laux sorry should have expanded my screen shot and comment, no i don't see that either:

image

I'm on c93ed2d96 which seems to be your latest commit.

@focusaurus
Copy link
Contributor

Just got caught up on the discussion and commit diffs. Looks OK to me. I'm going to try to get it working locally to verify functionality, but if that goes well will approve to merge.

@kieckhafer kieckhafer self-requested a review September 22, 2020 00:37
Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

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

This works as expected and described.

I'm personally not a huge fan of how you are required to choose a shop before you see any of the data: I think it might be confusing for a user to see nothing in their table if they don't know they need to choose a shop... That being said, I read through the discussions and it seems to be the consensus, so I'd say we push it out there and can always iterate over it.

Nice work, @loan-laux!!! 🎉

Once this gets merged in I'll put out a release with the updates!

@focusaurus focusaurus merged commit 4f3c448 into reactioncommerce:trunk Sep 22, 2020
@loan-laux
Copy link
Contributor Author

Wow, that was quick. Thanks @kieckhafer @focusaurus for the review. Are you guys okay with the way I implemented the URL prefix at the route level? I thought this to be a bit too rough around the edges, but I guess it can be improved on later on.

@kieckhafer
Copy link
Member

@loan-laux it works! this is still tagged beta so if it works and helps, I think getting it out there for other to try and iterate on is the best thing we can do. we don't have another solution at the moment so let's let the community play around and see if it fits everyones needs.

@focusaurus
Copy link
Contributor

+1 on landing this. It's a big PR and been open for months so best to get it landed and then continue forward with smaller tweaks. Thanks for this epic contribution, @loan-laux !

@bayareacoder
Copy link

bayareacoder commented Oct 14, 2020

in our custom Admin UI, we do similar: one shop selector top-left that is selected under Settings. Then all panels of settings apply to that shop, and all other screens (products, orders etc) as well. We store selected shop in localStorage so it is persistent and if localStorage is empty we just select the first shop as default so user does not need to select something before seeing data.

Screen Shot 2020-10-13 at 11 49 48 PM

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.

Add shop selector on dashboard for accounts with multi-shop access