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

Allow authors to customize whether a property is edited via a popup or not #695

Closed
LeaVerou opened this issue Feb 3, 2021 · 20 comments
Closed

Comments

@LeaVerou
Copy link
Member

LeaVerou commented Feb 3, 2021

Currently, the heuristic is that if the contents of a property are being edited, then the editor replaces the contents for the duration of editing, otherwise the property is being edited via a popup.

While the heuristic works for most cases, there are cases where it fails:

  • <span property="num" mv-edit-type="range">50</span> results in the contents being replaced by a slider, which is never desirable
  • <time property="date" datetime="15:00"></time> results in the time being duplicated into a popup, which is disruptive
  • SVG's <text> element stores its value in its contents, but we can't easily edit it inline.

image

We should probably adjust the defaults for these elements (we already disable popups for SVG <text>), although currently there is no way to set defaults based on the editor element. Should we also give authors control over this somehow?

Options:

  • An mv-popup attribute (with values auto, show, none?). What I don't like is that even though this is exclusively about editing, the name doesn't show this.
  • An mv-edit-popup attribute. That does show that it's about editing, but it means authors can never use the mv-edit-* syntax to set a popup attribute on the generated editor.
  • A set of class names, e.g. mv-edit-popup and mv-no-edit-popup. It seems a bit "wrong" to set functionality via class names though.
  • An extension on the mv-edit syntax, making it a key-value attribute. E.g. e.g. mv-edit="element: #foo, popup: show", mv-edit="element: #foo, popup: none". The single mv-edit="<selector>" syntax would still be supported, as a shortcut. This gives us an extension point for future metadata about editing, so I'm leaning towards that one for now. The problem is that in the general case it's quite hard to disambiguate if we have the options syntax, or a selector (e.g. foo:bar could be a key-value pair or a pseudo-class, foo, bar could be {foo: true, bar: true} or a foo, bar selector list). So we'll need to use a heuristic, which will likely be ok since nearly all usage of mv-edit currently is of the form #id.
@DmitrySharabin
Copy link
Member

I'm leaning towards the latter too. I like both the idea and the syntax.

The problem is that in the general case it's quite hard to disambiguate if we have the options syntax, or a selector

One thought:
What if instead of a comma (which can be used in a selector), we use, for example, a slash character to separate a selector from a set of properties? CSS heavily uses it in values for shorthand properties, e.g., grid-template.

So instead of mv-edit="element: #foo, popup: show, bar: baz" we could have mv-edit="element: #foo / popup: show, bar: baz". Or even shorter: mv-edit="#foo / popup: show, bar: baz".

And when we need to specify only properties, we can be explicit: mv-edit="element: initial / popup: show, bar: baz", or mv-edit="initial / popup: show, bar: baz", or simply mv-edit="/ popup: show, bar: baz" (I know, it has poor readability).

In all other cases, we are dealing with a selector.

@LeaVerou
Copy link
Member Author

LeaVerou commented Feb 3, 2021

What if instead of a comma (which can be used in a selector), we use, for example, a slash character to separate a selector from a set of properties? CSS heavily uses it in values for shorthand properties, e.g., grid-template.

We should have a consistent syntax across all key-value pair attributes though, the one parsed by Mavo.options()...

@DmitrySharabin
Copy link
Member

Oh, OK. Got it.

@LeaVerou
Copy link
Member Author

LeaVerou commented Feb 16, 2021

Another option would be a mv-edit.popup attribute.

Or, better, the opposite: Any attributes to be set on the editor would be mv-edit.* attributes, freeing up the mv-edit-* name space. This syntax-level difference also makes it clearer that this is not a predefined attribute, but is setting something arbitrary somewhere else. We could even adopt this naming scheme everywhere where we have attributes setting arbitrary properties or attributes somewhere (e.g. with the new mv-storage-* attributes, they could become mv-storage.* attributes). Main downside I can think of is we'll need to change a lot of existing code, and that periods in attributes are not something authors are familiar with (but I don't think it's hard to learn).

Actually, the more I think about this, the more I like this idea.

@DmitrySharabin
Copy link
Member

I like that idea too. And indeed, it's not hard to learn. Moreover, if an author has some experience with Angular, they familiar with the similar syntax in it.

Should we recommend plugin authors to sustain some naming conventions when adopting this scheme: concatenate multiple words, no camelCase, no hyphens, no underscores?
E.g., we'd expect mv-storage.sheetname instead of mv-storage.sheetName.

@LeaVerou
Copy link
Member Author

Should we recommend plugin authors to sustain some naming conventions when adopting this scheme: concatenate multiple words, no camelCase, no hyphens, no underscores?
E.g., we'd expect mv-storage.sheetname instead of mv-storage.sheetName.

I'm not sure, because they might be using it for a variety of things. E.g. I also want to support a .foo syntax for setting JS properties directly via expressions, and some of these may need to be camelCase.
Let's see what people do first, and form the design principle afterwards.

@karger
Copy link
Collaborator

karger commented Mar 3, 2021

A few thoughts.

  1. It's counterintuitive for me that I had to find the mv-edit syntax in the "properties" section of the docs instead of under UI customization.
  2. the rationale there for the mv-edit-* syntax is "you don’t want to entirely replace the element used for editing by default, but merely to set a few attributes on it". I worry that this represents an abstraction leakage. Suppose I do this, and then a future version of mavo changes the default editing control for a certain type. Then all of a sudden your set attributes might stop working.
  3. what if you set attributes on the editor using a meta tag inside the property to be edited? e.g.
    <span property="foo"><meta mv-editor attr1="value" attr2="val2"></span>
  4. Another alternative: use mv-editor-attr="value" to specify attributes on the editor, to distinguish from mv-edit attributes defining the editor itself.

@LeaVerou
Copy link
Member Author

LeaVerou commented Mar 3, 2021

A few thoughts.

  1. It's counterintuitive for me that I had to find the mv-edit syntax in the "properties" section of the docs instead of under UI customization.

Agreed, that's a documentation bug.

  1. the rationale there for the mv-edit-* syntax is "you don’t want to entirely replace the element used for editing by default, but merely to set a few attributes on it". I worry that this represents an abstraction leakage. Suppose I do this, and then a future version of mavo changes the default editing control for a certain type. Then all of a sudden your set attributes might stop working.

When you use that syntax, you're accepting that fluidity. Otherwise you'd just use mv-edit to provide the entire control.

  1. what if you set attributes on the editor using a meta tag inside the property to be edited? e.g.
    <span property="foo"><meta mv-editor attr1="value" attr2="val2"></span>

That seems clunky and doesn't work for every element due to content model limitations. Also, if you're worried that the practice is indicative of a leaky abstraction, any syntax for this is just as leaky.

  1. Another alternative: use mv-editor-attr="value" to specify attributes on the editor, to distinguish from mv-edit attributes defining the editor itself.

Oh that might be workable and let's us save the dot notation for JS properties exclusively. @DmitrySharabin what do you think?

@DmitrySharabin
Copy link
Member

What do you think of using mv-editor instead of mv-edit as well? The only functionality we are going to leave for the mv-edit attribute is to provide a custom editor. So why don't we just use mv-editor for this? Thus, we get the set of attributes that covers one feature. And I think it's rather intuitive.

@LeaVerou
Copy link
Member Author

What do you think of using mv-editor instead of mv-edit as well? The only functionality we are going to leave for the mv-edit attribute is to provide a custom editor. So why don't we just use mv-editor for this? Thus, we get the set of attributes that covers one feature. And I think it's rather intuitive.

Agreed it's intuitive, but then what does mv-edit do?

@DmitrySharabin
Copy link
Member

DmitrySharabin commented Mar 10, 2021

Agreed it's intuitive, but then what does mv-edit do?

I believe in that case, authors can specify whether a property is editable via a popup or not with the help of mv-edit, like so: mv-edit="popup" or mv-edit="via popup".

We can even provide other values, like opt-in or opt-out that address this issue: #595.

What do you think?

@LeaVerou
Copy link
Member Author

LeaVerou commented Mar 20, 2021

I'm not a huge fan of the idea of using a microsyntax in mv-edit to define a variety of settings instead of using separate attributes.

For now, it will be an alias of mv-editor with a deprecation warning. We'll see what to do with it in the future.

And I'll add an mv-edit-popup for what we're discussing here.

@DmitrySharabin would you be able to edit the docs and demos accordingly once I push these changes? 🙏🏼

Edit: pushed

@LeaVerou
Copy link
Member Author

LeaVerou commented Mar 20, 2021

So, for the popup, the syntax I'm thinking is an mv-edit-type attribute, with values auto, popup, self (for cases where the editing just happens via a behavior change on the element itself, like in <meter>), inline (editor nested in element). This would also allow for future values like e.g. dialog, whereas an mv-edit-popup attribute is not extensible.

Bikeshedding for names:

  • Attribute: mv-edit-type, mv-edit-ui
  • Values:
    • self could be element

What do you think?

@DmitrySharabin
Copy link
Member

DmitrySharabin commented Mar 20, 2021

@DmitrySharabin would you be able to edit the docs and demos accordingly once I push these changes? 🙏🏼

@LeaVerou Sure, Lea. Will do it now.

Edit:
Demos—mavoweb/mavo.io#108,
Docs—mavoweb/mavo.io#109 (updated pages: https://mavo.io/docs/ and https://mavo.io/docs/properties/),
The site pages—mavoweb/mavo.io#110,
Plugins—mavoweb/plugins#87.

And one more thing that fixes the behavior of the meter and the progress elements—#708.

Should we fix this as well (add mv-editor or replace mv-edit with mv-editor)?

"mv-attribute", "mv-default", "mv-mode", "mv-edit", "mv-permisssions",

@LeaVerou
Copy link
Member Author

Thank you so much Dmitry, fantastic job!!

I merged all PRs, let me know if I forgot anything!

Should we fix this as well (add mv-editor or replace mv-edit with mv-editor)?

"mv-attribute", "mv-default", "mv-mode", "mv-edit", "mv-permisssions",

Yes, I think so.

@DmitrySharabin
Copy link
Member

DmitrySharabin commented Mar 21, 2021

So, for the popup, the syntax I'm thinking is an mv-edit-type attribute, with values auto, popup, self (for cases where the editing just happens via a behavior change on the element itself, like in ), inline (editor nested in element).

Do I understand correctly that you suggest using mv-edit-type instead of mv-edit because the latter is deprecated although is still in use? If not, are you planning to use mv-edit somehow else?

Why am I asking? I'm not quite sure whether it's difficult to implement or not, but what if we could hide experimental features we are working on behind the flag as modern browsers do? So we could have something like the mv-flags attribute which authors can add to the root element of their app to play with upcoming features. Like so:

<E mv-app mv-flags="editor-ui">
<!-- Later on -->
<time property="date" datetime="17:00" mv-edit="self">The five o’clock tea time</time>

I believe that would let us perform migration and introduce breaking changes less painful. And we could already work with the new semantic of the mv-edit attribute.

@LeaVerou
Copy link
Member Author

No, I just don't think we should privilege mv-edit like this. mv-edit-type is one of many potential settings around how editing happens.

I like the flags idea, but we don't have that many users right now to need this. It would be good for the future however!

@karger
Copy link
Collaborator

karger commented Mar 23, 2021

mv-edit-type seems reasonable. another option would be mv-edit-mode. or for the specific purpose you are using, mv-edit-container

@LeaVerou
Copy link
Member Author

LeaVerou commented Apr 6, 2021

@DmitrySharabin we should probably add some documentation for this in the same page that mentions mv-editor and hte like

@DmitrySharabin
Copy link
Member

DmitrySharabin commented Apr 6, 2021

Sure, Lea. Will do it with pleasure.

Edit: Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants