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

Categorized tenants #10586

Merged
merged 16 commits into from
Feb 2, 2022
Merged

Categorized tenants #10586

merged 16 commits into from
Feb 2, 2022

Conversation

hishamco
Copy link
Member

@hishamco hishamco commented Oct 29, 2021

Addresses #10402

Screenshot 2021-10-29 182651

  • Sorting
  • Filtering
  • Pagination
  • Searching

@hishamco hishamco requested a review from jtkech October 29, 2021 15:30
@hishamco
Copy link
Member Author

I just added a Category to each tenant in tenants.json for simplicity and avoid any sort of breaking-changes

@agriffard
Copy link
Member

agriffard commented Oct 29, 2021

I don't see how we can have at the same time categories and pagination.

Ex: try.orchardcore.net currently has hundreds of tenants.

@hishamco
Copy link
Member Author

I don't see how we can have at the same time categories and pagination.

I add it to my TODO list, BWT the categories should not affect the sorting, filtering & pagination, the actual tenant should

@hishamco
Copy link
Member Author

@agriffard there's a little weired behavior during pagination, some categories are divided across the pages, I presume this is because the current ordering. It would be nice to check this PR against try.orchardcore.net or send me a user credentials PM if it's possible

Thanks

@deanmarcussen
Copy link
Member

Yeah I’m with @agriffard this will not work like this.

it’s also completely unlike any ui we have.

Suggest you look at the other search ui and make categories a filter in the controller. Workflows is as good example as any.

the button to go beside the sort button and list the categories

@hishamco
Copy link
Member Author

Suggest you look at the other search ui and make categories a filter in the controller.

This is a good option, which may simplify listing the projects, I suggest to display the category a longside the name and add it as a filter. If this sounds looks good I will change the PR now

@hishamco
Copy link
Member Author

I will do it like the category types in content items page

@hishamco
Copy link
Member Author

Screenshot 2021-10-29 213421

@hishamco hishamco requested a review from jtkech October 29, 2021 21:54
@deanmarcussen
Copy link
Member

deanmarcussen commented Oct 31, 2021

I don't like Not Categorized. For any existing user it would force them to go through and categorise all their tenants, just to get rid of an odd bit of text.

Categories (if we do it), should be optional.
So just use String.Empty, and None in the filter list (like we do elsewhere), and don't show anything if it is String.Empty`

And equally don't show the filter if there are no categories.

@hishamco
Copy link
Member Author

And equally don't show the filter if there are no categories.

So, how can we list the tenants that they don't have category?

And equally don't show the filter if there are no categories.

+1

@deanmarcussen
Copy link
Member

So, how can we list the tenants that they don't have category?

you would have an All. It would be the default.

We would never have a filter that forces people to put things into categories.

@hishamco
Copy link
Member Author

you would have an All. It would be the default.

No make sense, coz when you said None before ;) I will update the PR soon ...

@hishamco
Copy link
Member Author

hishamco commented Dec 8, 2021

Any updates on this?

@sebastienros
Copy link
Member

Can you update the screenshots if you removed the "not categorized" ?

@hishamco
Copy link
Member Author

Tenants categories

@hishamco
Copy link
Member Author

Please don't merge after I double check the merge conflict

@deanmarcussen
Copy link
Member

@hishamco you have missed my previous review.

Where I said that None shouldn't be showing.

If people aren't wanting to categorise their tenants (I can't think of any category I'd give mine, they are all of the same type), then having a ranom None tag showing up makes no sense.

i.e. Ok, with categorising, not ok with forcing it on users.

@hishamco
Copy link
Member Author

Oops, seems I forgot this one, I will update the PR soon ...

@hishamco
Copy link
Member Author

If there's no other comment, then the PR is ready to merge

@hishamco
Copy link
Member Author

Tenants categories

@hishamco
Copy link
Member Author

@deanmarcussen anything left here or can we approve then merge?

Copy link
Member

@deanmarcussen deanmarcussen left a comment

Choose a reason for hiding this comment

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

Two minor issues to fix. Otherwise looks good

Copy link
Member

@deanmarcussen deanmarcussen left a comment

Choose a reason for hiding this comment

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

You forgot to remove the default category - it's unnecesary

@hishamco
Copy link
Member Author

hishamco commented Feb 1, 2022

Seems I forgot to push a local commit from awhile ;)

@hishamco hishamco merged commit d2a06fc into main Feb 2, 2022
@hishamco hishamco deleted the hishamco/tenant-classification branch February 2, 2022 15:11
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.

5 participants