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

Sorted by dates by default #1257

Merged
merged 6 commits into from
Jul 13, 2021
Merged

Sorted by dates by default #1257

merged 6 commits into from
Jul 13, 2021

Conversation

tanmoyAtb
Copy link
Contributor

closes #1215

I kept the states in place, in case we want to have sorting functionality on the table columns in future

@render
Copy link

render bot commented Jul 12, 2021

@render
Copy link

render bot commented Jul 12, 2021

@render
Copy link

render bot commented Jul 12, 2021

@github-actions github-actions bot added the Type: Bug Fix Added to PRs if they are addressing a bug label Jul 12, 2021
@tanmoyAtb tanmoyAtb requested review from RyRy79261, FSM1 and Tbaut July 12, 2021 16:31
@tanmoyAtb tanmoyAtb added the Status: Review Needed 👀 Added to PRs when they need more review label Jul 12, 2021
tanmoyAtb and others added 2 commits July 13, 2021 17:07
@tanmoyAtb tanmoyAtb requested a review from Tbaut July 13, 2021 11:08
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

I just checked the UI, there's no sorting in the columns? Could we add it? Otherwise all the logic for the sorting direction etc isn't used.

@tanmoyAtb
Copy link
Contributor Author

I just checked the UI, there's no sorting in the columns? Could we add it? Otherwise all the logic for the sorting direction etc isn't used.

I did not put the sorting buttons into the column heads, as this was not in the issue scope,
The issue only asked for default sorting on dates,
But if thats something we want now, I could quickly add that in!

@Tbaut
Copy link
Collaborator

Tbaut commented Jul 13, 2021

My main concern is that this PR is a bit in between. Either we just build what the issue wants, and sort things per name in a hard-coded manner, or we copy/paste some older code, but then we make sure it's actually fully used. Right now all these useState have no actual use-case because there's no setter.

I guess it's a quick win to add the sorting in the header, but I personally wouldn't mind if you go the other route.

@tanmoyAtb
Copy link
Contributor Author

My main concern is that this PR is a bit in between. Either we just build what the issue wants, and sort things per name in a hard-coded manner, or we copy/paste some older code, but then we make sure it's actually fully used. Right now all these useState have no actual use-case because there's no setter.

I guess it's a quick win to add the sorting in the header, but I personally wouldn't mind if you go the other route.

All right, I totally feel you,
And the unused states were bugging me as well, so I added sorting on dates and size !

@tanmoyAtb tanmoyAtb enabled auto-merge (squash) July 13, 2021 18:05
@tanmoyAtb tanmoyAtb requested a review from Tbaut July 13, 2021 18:05
Copy link
Contributor

@FSM1 FSM1 left a comment

Choose a reason for hiding this comment

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

looking good 🎉

@tanmoyAtb tanmoyAtb merged commit acceb11 into dev Jul 13, 2021
@tanmoyAtb tanmoyAtb deleted the fix/tanmoy-date-sorting-1215 branch July 13, 2021 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Review Needed 👀 Added to PRs when they need more review Type: Bug Fix Added to PRs if they are addressing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Storage UI - Pins should be sorted by Created Date
3 participants