FEATURE: experimental fast edit (PR #14340)

Fast edit allows you to quickly edit a typo in a post, this is experimental ATM and behind a site setting: enable_fast_edit


Im aware this can have various edge cases and I’m opening this PR to see if we could decide that might good enough like this or if anyone has any idea of something which might be a deal breaker.

The fallback behavior of always showing edit and opening composer if we can’t fast edit is a brillant idea from @xfalcox !

OMG this is so cool!

I think the name of this file is no longer representative of what is going on here.

This is a really cool feature. I only left a couple of minor comments.

I think there should be a follow up commit to clean up the little usage of jQuery used in these files and name some things better.

Why are all these fields prefixed with a _?

I think the regex should be escaped first.

          _isFastEditable: false,
          _fastEditInitalSelection: null,
          _fastEditNewSelection: null
          _isFastEditable: true,
          _fastEditInitalSelection: quoteState.buffer.toString(),
          _fastEditNewSelection: quoteState.buffer.toString()

Why is this action name prefixed with a _?

Why is this action name prefixed with a _?

This probably does not work well when there is an “edit conflict”. I think it is fine for now, but it is something worth keeping in mind.

What happens if this.quoteState is null?

yes I thought about it, but not going to rename this in this PR, there’s already share which is unrelated too

private property not supposed to be set from the outside, eg it’s intended to show that the following shouldn’t be done:

{{quote-button _isSavingFastEdit=true}} 

It’s all virtual and not enforced but a well know patter in js world. In the future we might move to more advanced js class features, but don’t think we introduced this in the codebase yet.

No we don’t do this anymore because we will soon move to something which doesnt use set/setProperties, so one assignment by line will be easier to migrate

I don’t think it can actually be null, the current code doesn’t do any check of this kind and I’m not aware of any issue about this. In any case if it could be null it should probably be handled at the level where the function decides to show the quote button body, this ? is probably not needed here

same as before we don’t do this anymore

yes it was on my todo list I noticed issues which might be resolved by this