flarum / framework

Simple forum software for building great communities.
http://flarum.org/
6.39k stars 835 forks source link

Divide view into sub-elements #821

Closed yaserabdelaziz closed 4 years ago

yaserabdelaziz commented 8 years ago

I suggest dividing the return of each view function into sub-elements which will make it easier for extension developers to inject new components into the view, for example if I want to add a Thumbnail element to DiscussionListItem component it's hard and unstable to write code like this

extend(DiscussionListItem.prototype, 'view', function(vdom) {
  const discussion = this.props.discussion;

  vdom.children[2].children[0].children.unshift(
    <img className={'thumbnail'} src={discussion.thumbnail_url()}></img>
  );
});

it's more useful to reach vdom.children[2].children[0] directly by extending some function or accessing it using some key-value data-strucure.

franzliedke commented 8 years ago

Hmm, Toby and I were discussing implementing some helpers (possibly using strings with CSS-like selectors) that make common extension developer tasks like this easier.

From my perspective, that's what this topic should be about. (We're not going to change Mithril's vDOM structure, anyway.)

yaserabdelaziz commented 8 years ago

I hope CSS-like selectors would be unique and stable enough at the same time, multi-level selectors (e.g. #id_1 > #id_2) could break easily if id_1 wasn't there, as you said, in general, we shouldn't change Mithril's vDom structure.

franzliedke commented 4 years ago

@datitisev @clarkwinkelmann Would you guys say this is still a (generic) problem? We're definitely using the ItemList helper in more places now, which makes many use cases simpler, but not all of them...

clarkwinkelmann commented 4 years ago

Many methods were improved over the years so it's less of an issue.

We probably shouldn't keep such a generic issue open. Best to open issues about specific extensibility situations.

I never tried extending the discussion list as described in the OP so not sure if that's still an issue. One I recently noticed is the PostStream that uses an items array extensions can't modify without touching the vdom https://github.com/flarum/core/blob/0191babb05b0ffa1443447809686c1141a35b04f/js/src/forum/components/PostStream.js#L264

dsevillamartin commented 4 years ago

I think this issue is talking more about improving the vdom extending process (which is used when no other alternative is available - and there will always be a usecase for editing the vdom before rendering, else we'd have item lists everywhere and halt development).

You can see an example in FriendsOfFlarum's GeoIP that could benefit from a helper method with

https://github.com/FriendsOfFlarum/geoip/blob/c200c3edffb9cb935093a9c3028a66a4c405144f/js/src/forum/index.js#L52-L53

Helpers that check for failsafes to prevent erroring & use attributes, tags, class names (css selectors like mentioned above, or something similar) would be very helpful. However, they may not belong in core...

clarkwinkelmann commented 4 years ago

@datitisev

That's how I disable the password field in the user edit modal in the upcoming Wordpress extension:

        if (!this.kilowhatWordpressShowFields && items.has('password')) {
            const passwordField = items.get('password');

            if (
                Array.isArray(passwordField.children) &&
                passwordField.children.length > 1 && // .Form-group children
                passwordField.children[1] &&
                Array.isArray(passwordField.children[1].children) &&
                passwordField.children[1].children.length > 1 && // div children
                passwordField.children[1].children[1] &&
                Array.isArray(passwordField.children[1].children[1].children) &&
                passwordField.children[1].children[1].children.length > 0 && // label children
                passwordField.children[1].children[1].children[0] &&
                passwordField.children[1].children[1].children[0].tag === 'input'
            ) {
                if (!passwordField.children[1].children[1].children[0].attrs) {
                    passwordField.children[1].children[1].children[0].attrs = {};
                }
                passwordField.children[1].children[1].children[0].attrs.disabled = true;
            }
        }

It would work with less conditions, but if any other extension touches something, there would be a risk of breaking the UI :sweat_smile:

A css-selector based vdom edit method would be great. I think it would have its place in core.

dsevillamartin commented 4 years ago

@clarkwinkelmann Yeah, that's a reason why I usually use Array.prototype.find instead of a specific index (unsupported in IE LOL).

franzliedke commented 4 years ago

Okay, then we need a ticket. Given the discussion in this one and the title are somewhat misleading, could one of you open a new ticket about VDOM manipulation helpers, with a few example use-cases?

clarkwinkelmann commented 4 years ago

Opened flarum/issue-archive#228

franzliedke commented 4 years ago

Thanks!