Arvados

Reviewing 84decc8. The code here is very clean and easy to follow; thank you.

Please have FoldersTest#setup call super. This will be functional when you merge, because master implements IntegrationTest#setup to reset the Capybara session for improved isolation.

Is there a way to unparent a folder? In other words, make it a direct child of the "My Folders" pseudofolder? If this is possible, it wasn't clear from the interface.

The rest is more stylistic and needn't block a merge: FoldersController#index defines inner methods with def. I learned recently that this is apparently discouraged Ruby style, because the way Ruby scopes work, you're actually defining an instance method, which can lead to surprises later. To illustrate:

> class Foo
>   def bar
>     def baz
>       42
>     end
>     baz + 2
>   end
> end
 => :bar
> f = Foo.new
 => #<Foo:0x000000013403e0>
> f.baz
NoMethodError: undefined method `baz' for #<Foo:0x000000013403e0>
        from (irb):17
        from /home/brett/.rvm/rubies/ruby-2.1.1/bin/irb:11:in `<main>'
> f.bar
 => 44
> f.baz
 => 42
> g = Foo.new
 => #<Foo:0x0000000129e540>
> g.baz
 => 42

Recommended style is to define the method directly on the class, or if you need a closure, use lambda.

For the Collections file tree, one reason I was kind of adamant about using the list tags and styling them as desired is because I thought it would be better for accessibility. Someone using a screen reader would be empowered to navigate the list more quickly, skip entire lists, etc. I presume we have lots of accessibility obstacles right now, but I wanted to avoid introducing new ones at least. I realize it comes with its own problems like funky highlighting, but I'll throw this out there for consideration. Again, I don't consider this an obstacle to merging.

Thanks.

Brett Smith wrote:

Reviewing 84decc8. The code here is very clean and easy to follow; thank you.

Please have FoldersTest#setup call super. This will be functional when you merge, because master implements IntegrationTest#setup to reset the Capybara session for improved isolation.

Aha, I didn't realize I was being sneaky but apparently I was. It doesn't override setup, it uses "setup do ..." which is shorthand for "super; ...", aka "add this to the list of setup tasks". (Looking at docs it seems "before do ..." is the canonical form and setup is an alias.)

Is there a way to unparent a folder? In other words, make it a direct child of the "My Folders" pseudofolder? If this is possible, it wasn't clear from the interface.

Yes, you should be able to do this, but currently your own name is your top-level folder. Perhaps just need to show "My Folders" as the top level folder (owner_uuid=user_uuid under the hood) rather than a "My Folders" non-item + "Your Name Here" top-level folder. Sound right?

The rest is more stylistic and needn't block a merge: FoldersController#index defines inner methods with def. I learned recently that this is apparently discouraged Ruby style, because the way Ruby scopes work, you're actually defining an instance method, which can lead to surprises later. To illustrate:

Recommended style is to define the method directly on the class, or if you need a closure, use lambda.

Hm, yes, in fact that's not the scope I was expecting. Fixed in c95e276

For the Collections file tree, one reason I was kind of adamant about using the list tags and styling them as desired is because I thought it would be better for accessibility. Someone using a screen reader would be empowered to navigate the list more quickly, skip entire lists, etc. I presume we have lots of accessibility obstacles right now, but I wanted to avoid introducing new ones at least. I realize it comes with its own problems like funky highlighting, but I'll throw this out there for consideration. Again, I don't consider this an obstacle to merging.

Good point. I'm OK with leaving it as a "known issue" right now. (Would "div within div" help a screen reader? That would give us more options for highlighting styles, while still expressing the nested structure. Surely we'll want something nested in any case when we add expand/collapse.)

Reviewing c95e2762

Tom Clegg wrote:

Brett Smith wrote:

Please have FoldersTest#setup call super. This will be functional when you merge, because master implements IntegrationTest#setup to reset the Capybara session for improved isolation.

Aha, I didn't realize I was being sneaky but apparently I was. It doesn't override setup, it uses "setup do ..." which is shorthand for "super; ...", aka "add this to the list of setup tasks". (Looking at docs it seems "before do ..." is the canonical form and setup is an alias.)

Not sneaky, I should've caught that. Thanks for explaining. This does seem like a better approach overall.

Is there a way to unparent a folder? In other words, make it a direct child of the "My Folders" pseudofolder? If this is possible, it wasn't clear from the interface.

Yes, you should be able to do this, but currently your own name is your top-level folder. Perhaps just need to show "My Folders" as the top level folder (owner_uuid=user_uuid under the hood) rather than a "My Folders" non-item + "Your Name Here" top-level folder. Sound right?

Yeah, that was what tripped me up. I understood why "My Folders" and "Shared With Me" were distinct, but I didn't understand it wasn't possible to go between "My Folders" and "[account name]." I think your proposal would be a great improvement if it's not too difficult.

For the Collections file tree, one reason I was kind of adamant about using the list tags and styling them as desired is because I thought it would be better for accessibility. Someone using a screen reader would be empowered to navigate the list more quickly, skip entire lists, etc. I presume we have lots of accessibility obstacles right now, but I wanted to avoid introducing new ones at least. I realize it comes with its own problems like funky highlighting, but I'll throw this out there for consideration. Again, I don't consider this an obstacle to merging.

Good point. I'm OK with leaving it as a "known issue" right now.

I think that's good. Later on maybe we can figure out one solution for styling trees generally, and then apply that to all the ones we have. (One idea that came to me was a sort of hybrid of my approach and yours: use semantic list markup, but style away all the list margins and padding, and instead put an empty div at the beginning of each li to visually present the hierarchy.)

(Would "div within div" help a screen reader? That would give us more options for highlighting styles, while still expressing the nested structure. Surely we'll want something nested in any case when we add expand/collapse.)

Off the top of my head, I don't really know. I'm doubtful, because the whole point of divs is that they don't provide any semantics. But I know some people I could ask.

At 7c8e4a8

I think 7c8e4a8d is good to merge. Thanks.

Reviewing 52bf3c092. It looks good to merge to me. I'm a little nervous that the API server and Workbench have to duplicate information about what data can go in folders—I'm worried that we'll see weird bugs later if they diverge—but I don't have a better idea either. I'm not even sure where you might document this and be relatively confident that people could see it when they worked on this, starting from either end.