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

feat: dropdown in project search to order projects #501

Merged
merged 1 commit into from
Jul 4, 2019

Conversation

vfried
Copy link
Contributor

@vfried vfried commented Jul 2, 2019

With dropdown in project search to order projects by name, created date, and updated date.

With sorting included (Ascending - Descending) --> I'm not sure this button icons are the best, if you have better ones i can change them.

image

@vfried vfried requested a review from a team as a code owner July 2, 2019 15:33
@vfried vfried self-assigned this Jul 2, 2019
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

Very nice! There are a couple of comments I added, but it's working fine!

Another minor detail: it would be nice if clicking on the current value on the Order By dropdown menu doesn't reload it.

src/project/list/ProjectList.container.js Outdated Show resolved Hide resolved
src/project/list/ProjectList.container.js Outdated Show resolved Hide resolved
src/project/list/ProjectList.container.js Outdated Show resolved Hide resolved
@lorenzo-cavazzi
Copy link
Member

I was thinking if it makes sense to change the "Order By" text on the button with the current chosen element. While using it, I noticed it is not instantly clear which is the ordering element. But I am not sure if this will make the button functionality less obvious

@vfried
Copy link
Contributor Author

vfried commented Jul 3, 2019

I was thinking if it makes sense to change the "Order By" text on the button with the current chosen element. While using it, I noticed it is not instantly clear which is the ordering element. But I am not sure if this will make the button functionality less obvious

I thought about this too and it could be done but I decided to do it this way because as you said it wouldn't be clear what the dropdown does otherwise. I also tryied putting order by and the selected action but it was too much text.

I think if in the future we have aditional search functionalities/filters we could improve the way it looks by transforming this into a full search bar line or something like what google does.

Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

Very nice!

There is only one other minor detail that I noticed on "ProjectList.present", and a couple of this.model.set on the "ProjectList.container" that can easily be removed in favor of the equivalent "ProjectList.state" functions -- I didn't mark them explicitly on the first review, sorry for that

src/project/list/ProjectList.container.js Outdated Show resolved Hide resolved
src/project/list/ProjectList.container.js Outdated Show resolved Hide resolved
src/project/list/ProjectList.present.js Outdated Show resolved Hide resolved
@lorenzo-cavazzi
Copy link
Member

I thought about this too and it could be done but I decided to do it this way because as you said it wouldn't be clear what the dropdown does otherwise. I also tryied putting order by and the selected action but it was too much text.

I think if in the future we have aditional search functionalities/filters we could improve the way it looks by transforming this into a full search bar line or something like what google does.

Right, it makes sense to leave it as it is now and improve it in the future when we need

@ciyer
Copy link
Contributor

ciyer commented Jul 3, 2019

Very nice implementation!

Would it be possible to sort "Your Projects" and "Starred Projects" by update date?

@vfried
Copy link
Contributor Author

vfried commented Jul 4, 2019

Very nice implementation!

Would it be possible to sort "Your Projects" and "Starred Projects" by update date?

Done!

BTW: i tested and the pipeline works for me. ==> I first have to create a trigger to the use the trigger token to run the pipeline. (I cannot start a pipeline without a token).

@lorenzo-cavazzi lorenzo-cavazzi self-requested a review July 4, 2019 12:38
lorenzo-cavazzi
lorenzo-cavazzi previously approved these changes Jul 4, 2019
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

Working for me. It probably needs to be rebased before it can be merged.

Would it make sense (an be easy) to add this search also to starred and personal projects?

@vfried
Copy link
Contributor Author

vfried commented Jul 4, 2019

Working for me. It probably needs to be rebased before it can be merged.

Would it make sense (an be easy) to add this search also to starred and personal projects?

It makes sense but it wouldn't be that easy... since the project search and the starred/your projects are different components. So we would need to do this in a separate issue/pr.

@vfried vfried merged commit f1a5191 into master Jul 4, 2019
@ciyer ciyer added this to the 0.6.0 milestone Jul 19, 2019
@ciyer ciyer deleted the 475-order-projects branch August 20, 2019 07:09
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.

3 participants