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

UI: Add allocation directory sorting #5914

Merged
merged 125 commits into from
Jul 23, 2019

Conversation

backspace
Copy link
Contributor

@backspace backspace commented Jul 2, 2019

You can try this out here. A GIF of it in action:

closer-arrows

When sorting by size, directories are sorted by name, as size isn’t displayed.

This includes a change to the positioning of sort arrows for all tables, where this:

image

becomes this:

image

Because the directory table includes sortable right-aligned columns, having the arrow at the other edge of the cell sometimes makes it ambiguous which column it’s referring to. Moving the arrow beside the label is clearer. It’s also helpful in the directory viewer’s left-aligned “Name” column, which is comparatively quite wide, so the arrow otherwise was far away:

image

backspace added 30 commits June 21, 2019 15:14
This doesn’t work in every instance; sometimes when I refresh
there’s a failure. I’ll have to tune the models created to
eliminate these occasional problems.
This will surely break under some circumstances; refinement
to come.
This isn’t truly necessary but I think it makes it easier
to understand the test.
backspace added 19 commits July 2, 2019 15:22
This has a formatting problem where the sorting arrow overlaps
with the heading label, but I’ll handle that separately.
Since directory sizes aren’t displayed (as they aren’t useful),
this sort falls back to being by name for directories.
# Conflicts:
#	ui/app/controllers/allocations/allocation/task/fs.js
#	ui/app/templates/allocations/allocation/task/fs.hbs
#	ui/mirage/config.js
#	ui/tests/acceptance/task-fs-test.js
#	ui/tests/pages/allocations/task/fs.js
I almost never write controller tests but maybe this is
more suitable than an acceptance test? 🧐
This change is unnecessary now that there’s specific data for
the sorting test.
This opens the way for right-aligned columns to have a
similar appearance, with the arrow immediately before
the column heading.
Without this, changing the sort causes the column widths
to change as well.
I thought this didn’t work yesterday…?? But it seems preferable
to having the arrows take up space when tables get narrow.
This includes a merge, oops ☹️
This feels a bit weird so far from the .is-selectable
declarations, but I couldn’t get it to apply down there.
@backspace backspace requested review from DingoEatingFuzz and a team July 4, 2019 15:27
@backspace backspace marked this pull request as ready for review July 8, 2019 14:25
Copy link
Contributor

@DingoEatingFuzz DingoEatingFuzz left a comment

Choose a reason for hiding this comment

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

Great job adopting an existing pattern to this page!

As for whether the acceptance test or controller test is preferred, I'm leaning towards acceptance test, but I don't feel strongly about it.

ui/tests/acceptance/task-fs-test.js Show resolved Hide resolved
@backspace
Copy link
Contributor Author

I removed the controller test. Thanks!

@backspace backspace merged commit f046ec3 into f-ui/alloc-fs Jul 23, 2019
@backspace backspace deleted the f-ui/alloc-fs-directory-sort branch July 23, 2019 20:37
@github-actions
Copy link

github-actions bot commented Feb 6, 2023

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants