Internalize dialogs to data browser

[matthewma7]

This PR involves a few related things so I create one PR instead of multiple. But each commit individually should be easy to review.

The first commit improved the girder-breadcrumb component. There always has been console warnings complaining about deprecated slot usage with v-breadcrumb. This commit fixes that issue and prepared for the new slot syntax. This commit also fixes a couple of issues introduced in the previous PR.

The second commit internalized the Upload and New Folder dialog to DataBrowser. The reasoning is that three times I use DataBrowser, I always wanted UpsertFolder and Upload to show just like they are in the demo app, but I needed to create the dialogs, hooking up the callbacks correctly, handling show state myself three times. By internalizing the dialog, the saved effort is huge. (See code reduction of demo/App.vue https://github.com/girder/girder_web_components/pull/125/files?file-filters%5B%5D=.vue) But this does make things a little bit more complex within the component, but the complexity is hidden from the end user of the component, as other good libraries do.

The third commit makes boolean props always negate its default value, so their default value can be deduced from the naming itself, and that when using the props, only the name is needed. This commit also removes a couple of unused props. e.g. one of the props was called newFolderEnabled but it has a default value of true, so, it has to be set as :new-folder-enabled='false' to use it. It has been renamed to 'noNewFolder', to indicate it's default value is false, and when using, only need the mention the prop without value <girder-data-browser no-new-folder />

Overall, this PR improves GWC usability.

Resolves https://github.com/girder/girder_web_components/issues/124 and https://github.com/girder/girder_web_components/issues/123

[matthewma7]

This looks good to me😉. How about others?

[matthewma7]

I really believe this is a good change. Could you guys take a look?

@subdavis @zachmullen

[subdavis]

Hey @matthewma7

I definitely understand what your goal was with these changes, and it makes the user experience of using GWC demo well. That being said, I'm concerned about the flexibility and code isolation it sacrifices.

On another note, I would prefer that the prop names also remain unchanged. All of the negative variable names make the code harder to read and reason through. The double negatives especially hurt readability. I think it's a pretty common best-practice to avoid such naming, and the cost of finding the props I need to set as positive is very low.

How do others feel about this?

[matthewma7]

@subdavis The props naming change is something I and @jeffbaumes noticed separately. Most of Vuetify's boolean props are set up this way (negate default value), even standard HTML boolean attribute works this way for input (disable, readonly), video (controls, autoplay, muted).

About the flexibility for this case specifically, I don't think it's too much to provide two dialogs when the user asks for specifically for New Folder or Upload buttons. If the consumer wants total flexibility, he/she still have the slot for that. Could you explain what do you mean by code isolation in this case?

[subdavis]

@matthewma7 I agree that the current props are unconventional, and I would be fine with changing the defaults to negative values. What I don't want to have is negative-named variables such as noSelect. I would prefer that selectEnabled defaulted to false and to turn on that behavior with <girder-data-browser select-enabled> because I don't think the double-negative code reads well.

As an example, I think the following lines read poorly:

• !loading && !noMoreItem && searchResults.length > maxQuickResults
• `this.noSelection || ['root', 'users', 'collections'].indexOf(type) !== -1

I don't think it's too much to provide two dialogs when the user asks for specifically for New Folder or Upload buttons.

I am not comfortable with proxying props through one component to another as with uploadProps, and I also don't think code related to Upload and UpsertFolder's apis should be entangled with the data browser. Vue slots exist so that we don't have to do things like that.

[zachmullen]

+1 for avoiding double negatives. (HTML is an abomination)

[matthewma7]

@subdavis Hm, I see what do you mean now. Ok, without double negative, the default feature will be the most minimal set, which does make sense itself 👌. ===Update=== I changed all the double negatives except the root prop of DataBrowser, if root is not allowed by default, it will make DataBrowser require a location prop.

[matthewma7]

I see props pass through is a thing, but a consumer doesn't need to do anything if they just want the default behavior. Vuetify menu-props of Select is one example. Easy to use for consumers sometime does mean complexity behind the scene, the ultimate example is the Google search bar (Just an <input type='text' /> and a <input type='submit' />, right? 😜). Vuetify does tons of hacky stuff internally (Take how dialog not being rendered to the default location on DOM for example, here and here), but its component interface is simple to use.

[danlamanna]

Easy to use for consumers sometime does mean complexity behind the scene, the ultimate example is the Google search bar (Just an and a , right? 😜). Vuetify does tons of hacky stuff internally (Take how dialog not being rendered to the default location on DOM for example, here and here), but its component interface is simple to use.

I'm pretty removed from this PR, but I just wanted to chime in here. Maybe we want to take on the extra burden of complexity here for the sake of an easy component interface, maybe not. But implying that Vuetify does it so it's okay for us doesn't sit right with me. We're not Vuetify, we may or may not have the contributor/user base to support this particular complexity. We should allow complexity to the level we're comfortable with maintaining, and we should use upstream interfaces as a guide for making GWC familiar/intuitive to existing Vuetify users if we can afford it.

[matthewma7]

I'm pretty removed from this PR, but I just wanted to chime in here. Maybe we want to take on the extra burden of complexity here for the sake of an easy component interface, maybe not. But implying that Vuetify does it so it's okay for us doesn't sit right with me. We're not Vuetify, we may or may not have the contributor/user base to support this particular complexity. We should allow complexity to the level we're comfortable with maintaining, and we should use upstream interfaces as a guide for making GWC familiar/intuitive to existing Vuetify users if we can afford it.

The examples I used is too extreme. I don't think it's possible to understand what Vuetify is doing with dialog by just browsing the code, but the complexity I am introducing here is understandable by just browsing the code, without stepping the code or asking me. The point I am trying to make is easy to use consumer interfaces do tends to introduce complexity in the box.

[jeffbaumes]

The tradeoff between internal and external usability is a great one to discuss, since we should try to accommodate both but make an intentional choice when they conflict. I like the in the box complexity vs. external usability that @matthewma7 talks about, and @danlamanna's comments as well.

Indeed, I'm the one who suggested that we use default-opposing names for flags, to avoid requiring setting false flags. That is, use "disabled" instead of "enabled=false". But I agree that names like "noSelect" are too obviously negative and should be discouraged because of confusing double-negatives.

As @matthewma7 has said, we can solve both problems by having more features turned off by default. This avoids needing a falsy/negative name.

@zachmullen HTML is not great in many cases, but the fact is people know HTML, and will be using HTML to use this library (even pug has shorthand for attribute flags), so we should accommodate where reasonable to conform to expectations.

I'll note that in the case of an extreme conflict (a negative name is much more usable externally, and a positive name is much more usable internally), we can simply have a computed property that negates the value for internal use:

computed: {
  selectionEnabled() { return !this.selectionDisabled; }
}

Not sure we ever would have that extreme of a conflict, but there it is.

[subdavis]

So it seems like there's still a bit of fuzziness around this PR's approval.

1. I approve the prop renaming.
2. I do not approve internalizing UpsertFolder and Upload to data browser. Vue.js provides a convention, Slots, for this kind of composition as part of its core offering. We should embrace the Vue.js convention and avoid re-inventing a mechanism to pass props through a child to a grandchild. I support making life easier for other developers / consumers / our future selves, but I think this PR attempts to protect the user from core Vue concepts in a way that isn't helpful. Finally, I don't like how this PR required you to make the unit tests less specific by removing props. The tests failed when you made these changes, which was good, and now they won't fail in the future when the API changes again. The tests also require you to put UpsertFolder and Upload test logic into your DataBrowser.spec

Here are the options I see for moving forward (In the order of what I'd prefer).

1. I actually do believe an out-of-the-box DataBrowser with Upload and UpsertFolder is nice. I've never needed one (which is perhaps why our opinions differ) but I understand why you might. I propose a new folder ./src/examples (needs a better name) where we can put wrapped compositions of other components, which we can document as fragile and untested. This will allow us to keep DataBrowser nice and tidy but still allow us to have a magic wrapped component. I still reject the idea of an uploadProps prop, so it would be best to just pick some sane defaults. I also support removing the newItemEnabled, uploadEnabled and newFolderEnabled props and the associated buttons, and leaving the header widget slot empty.
2. @jeffbaumes or @zachmullen make a final ruling to accept or reject these changes wholesale.

Whether it gets merged or not, I think this was valuable for discussion. Thanks @matthewma7!

[zachmullen]

Let's separate out the prop renaming into a separate PR since it seems uncontroversial, and get that merged in.

After years of listening to our team's concerns about product stewardship, ownership, and cost of maintenance of tools like these that don't always have obvious project funding sources, I'm inclined to defer to @subdavis in his role of GWC product pig. Ultimately the pigs bear the cost of changes like these for the life of the code, and this cost appears to be intolerable; I want to make sure we are avoiding what I call "burden creep", in which downstream pain and burdens migrate upstream into repositories that have smaller bases of funding and developers, where they become "chronic pain" rather than pain that is bound to project lifecycles.

To be clear: I think internalizing these things (as a default) does add value, and I applaud @matthewma7 's effort to make the component easier to use. This is not a question of whether the change is beneficial, but simply of whether the benefits outweigh the cost. Very early on in the life of this repository I tried to impart some principles/guidelines to @subdavis in particular and I think he's adhering to them here: YAGNI, KISS, and low TCO.

[jeffbaumes]

Thanks @zachmullen, agreed. At this point in GWC's life, I'd cast my vote toward core repo ease of maintenance over external ease of use. Realistically we will never be at Vuetify's level of adoption (by at least a few orders of magnitude) simply due to the scope of this repo, and we should weigh changes accordingly toward maintainability. Perhaps as @subdavis says there is a way to specify untested but convenient components, but that's a separate discussion. It's always hard to accept a change that you know will make you add repeated code to multiple projects. It feels wrong, and often for good reason, but regardless it is sometimes the right decision from a cost/benefit perspective.

[zachmullen]

In case not everyone knows the fable

[matthewma7]

we should embrace the Vue.js convention and avoid re-inventing a mechanism to pass props through a child to a grandchild.

Vue.js is a core library, of course it doesn't many conventions. Vuetify does have prop delegate pattern, Vuetify also introduced patterns like Activator slot. We are using Vuetify and is asking the user to know Vuetify, I don't see this add any new pattern.

The burden of adding UpsertFolder and NewFolder already happened multiple times in multiple projects I and @jeffbaumes worked on.

I still reject the idea of an uploadProps prop, so it would be best to just pick some sane defaults

Without the prop, the user won't be able to control what kind of file to allow or get notified when upload finish. No matter how customization is supported, with uploadProps or other delegate props, these options are needed.

The tests failed when you made these changes, which was good, and now they won't fail in the future when the API changes again. The tests also require you to put UpsertFolder and Upload test logic into your DataBrowser.spec

I don't understand what cost does the change impose on a unit test, we do shallow mount and it's a dialog or a div is the same thing. Why did you say they won't fail in the future?

[matthewma7]

I think this PR attempts to protect the user from core Vue concepts in a way that isn't helpful.

I don't believe the user care about the standard Vue concept more than they just want to have upload and new folder.

[matthewma7]

I didn't make these changes simply because I wanted to mimic what Vuetify does, I have been using the Data Browser multiple times, and each time I have been literally copying the example/App.js, the existence of the need prompt me to make this change.

[matthewma7]

@zachmullen

I want to make sure we are avoiding what I call "burden creep", in which downstream pain and burdens migrate upstream into repositories that have smaller bases of funding and developers

I am not aiming to take the burden to easy downstream projects in general. I got the idea that we don't want to put ourselves in such a place. But I think we could just talk about this internalizing these two dialogs specifically.

@subdavis I might understand incorrectly what you mean by

I propose a new folder ./src/examples (needs a better name) where we can put wrapped compositions of other components

Are you saying the downstream user is able to reuse the code instead of copy and paste the code? If so, Could the directory be ./src/snippet instead of ./src/example?

[subdavis]

Are you saying the downstream user is able to reuse the code instead of copy and paste the code? If so, Could the directory be ./src/snippet instead of ./src/example?

Yes, it would be reusable but keep the cruft out of core components. uploadProps, upsertFolderProps still makes me uncomfortable, but it's more reasonable in this "snippet" context than in DataBrowser.vue.

[matthewma7]

Yes, it would be reusable but keep the cruft out of core components. uploadProps, upsertFolderProps still makes me uncomfortable, but it's more reasonable in this "snippet" context than in DataBrowser.vue.

So, would the example/app.vue use the wrapped version? The example has so much influence on how to use would use these components. If not the example/App.vue would still take the burden creating the dialog and delayed the closing of dialog.

[zachmullen]

We perceive a negative value in merging it, that's the issue. There are two risks causing concern:

1. Increased internal complexity to maintain in the future
2. Propagation of not-so-good practices w.r.t. Vue.js

Can we come up with an alternative implementation using slots that helps alleviate the burden on downstreams? Would that address both of those risks?

[matthewma7]

@zachmullen Technically, I think slot is used when the parent component wants to render something, and here we are making it easier so the parent doesn't need to render something. Just to clarify, are these code the code we have concerns? I can change it to not use the pass-through prop pattern, but easier to maintain the explict pattern. Instead of upload-props, it would be upload-accept, upload-multiple, etc?

[subdavis]

The implementation using slots is exactly what already exists in demo/App.vue. It is my understanding that the issue @matthewma7 wants to solve is the duplication of the following:

postUpload() {
  // postUpload is an example of using hooks for greater control of component behavior.
  // here, we can complete the dialog disappear animation before the upload UI resets.
  this.$refs.girderBrowser.refresh();
  this.uploader = false;
  return new Promise(resolve => setTimeout(resolve, 400));
},
postUpsert() {
  this.$refs.girderBrowser.refresh();
  this.newFolder = false;
  return new Promise(resolve => setTimeout(resolve, 400));
},
uploadDest() {
  if (this.location) {
    return this.location._modelType === 'folder' ? this.location : this.folder;
  }
  return null;
},

v-dialog(v-model="uploader", full-width, max-width="800px")
  girder-upload(
      v-if="uploadDest",
      :dest="uploadDest",
      :post-upload="postUpload",
      multiple="multiple")
v-dialog(v-model="newFolder", full-width, max-width="800px")
  girder-upsert-folder(
      v-if="nonRootLocation(location)",
      :key="location._id",
      :location="location",
      :post-upsert="postUpsert",
      @dismiss="newFolder = false")
v-container(v-show="!loggedOut", fluid)
  v-layout(row, wrap)
    v-flex.mr-2(grow)
      v-card
        girder-data-browser(
            ref="girderBrowser",
            :location.sync="location",
            :select-enabled="selectEnabled",
            :draggable="dragEnabled",
            :new-item-enabled="newItemEnabled",
            :new-folder-enabled="newFolderEnabled",
            @click:newitem="uploader = true",
            @click:newfolder="newFolder = true",
            @selection-changed="selected = $event")

IMO, this code does not need to be wrapped. If a consumer wants a data browser, an uploader, and a folder creation component to work together, they can create those things individually and implement the three methods required to orchestrate them if they want to do the dialog dance.

Matthew wants to wrap these in <data-browser, :location...>. The trouble with this is the upload and upsert components have their own props, like "multiple" and "accept" which should be user-configurable. Matthew is proposing <data-browser, :location=location, :upload-props="{ multiple: true, accept: 'mp4' }"/>

I don't consider the trade-off worthwhile to support here, but this is the crux of the discussion.

[matthewma7]

Yes, that summarizes the issue. @subdavis

The reason I did the pass-through is to let the user have control of Upload and upsertFolder, but it is the passing through pattern itself that caused our concern. but the purpose of internalizing these components. Right? Now, looking at the props of UpsertFolder and Upload, since the data browser is controlling their location, there are not many props left to be customized, allow me to change it to easier to maintain and explicit pattern and let's take a look.

[subdavis]

Can we not do the snippet thing? That would produce exactly the result you want and keep the code apart.

If you brings Upload and Upsert into databrowser, then all of DataBrowser's private methods (like "fetchPaginatedRows") have access to those components without having to go through DataBrowser's public API (props/events). I don't want a future where the Upload inside DataBrowser can have special side-effects because of some new private API that springs up between the two. I don't even want to have to look out for that.

[matthewma7]

@subdavis The reason to bring DataBrowser and Upload, UpsertFolder together is simply that they always work together. The existing code that only provides one button called 'Upload', and 'New folder' doesn't provide much value than the free form slot, which is next to them. The DataBrowser is not a genetic presentation component, it's Girder data browser, so it's natural that a Girder data browser is able to do Girder upload, and create Girder folder, and in the following PR, be aware of Girder access control. Without those features. It's would be simple and clean, but not too useful as Girder data browser.

[matthewma7]

Updated. Now it's just a handful of copy-paste props to pass to child props.

[subdavis]

I agree, these things work intimately together and are in many ways inseparable. I'm raising a software quality rather than a project scope concern. The components can remain tightly coupled and still interact through rigorously defined interfaces.

For example, in this PR, there's nothing stopping the postUpsert method from reaching into this.pagination. postUpsert doesn't have access to the private data and methods of DataBrowser elsewhere (it does through $refs, but we strongly discourage their general use). If you want to do this in your own project code, you can, and you may or may not get bitten. But for the library itself, it's best to avoid reaching inside other components and looking at their state.

With the snippet approach, the components's interactions are restricted by their officially exposed behaviors. This has the dual benefits of serving as additional library documentation ("This is how the things work together") and can also reveal places where the official interface lacks flexibility.

We clearly have some conflicting needs, and the snippet approach caters to those needs in a reasonable way that we both get value from.

Finally, thanks for the update. I think I've miscommunicated my concern. Changing a single forwarded prop object to many individual forwarded props doesn't really address the issues above.

[matthewma7]

For example, in this PR, there's nothing stopping the postUpsert method from reaching into this.pagination. postUpsert doesn't have access to the private data and methods of DataBrowser elsewhere (it does through $refs, but we strongly discourage their general use).

Could you explain this with an example? Is this the only concern you have with copy paste props?

[matthewma7]

Ok ok.

So, do we still want the prop renaming part of this PR or just the first bugfix commit?

[matthewma7]

Closing in favor of the snippet approach with https://github.com/girder/girder_web_components/pull/131 Bugfixes, improvement and boolean prop refactor has been moved to https://github.com/girder/girder_web_components/pull/133