Open herbdool opened 1 year ago
I really like this suggestion 👍🏼
...but for the sake of playing the devil's advocate:
Please see https://github.com/backdrop-contrib/fast_token_browser/issues/3 - I do think the UX should be improved before it is merged into core.
As @klonos is the devil's advocate, I should probably be the angel's advocate and say that core breaks if you have a lot of tokens which this fixes. If we can get the UX improved, then it is not a new feature per se but a replacement of an existing feature.
the token seems to always be appended to the end of the existing text in the target text field/area
This should be pretty simple to fix - coming up with a combination of the behaviors of the current token browser and FTB
Yes, if we managed to address that UX issue, I'd be happy for this to get into core. The way that FTB works (loading tokens via AJAX as needed) sounds like how things should be working to begin with.
...I've added the [PS]
(Performance & Scalability) "tag" in the title of this issue, as per https://docs.backdropcms.org/documentation/how-to-use-the-core-issue-queue, based on the fact that the implementation of loading tokens in FTB via AJAX is a performance improvement.
@klonos - the latest release with the changes from @argiepiano , I believe, have addressed your concerns.
Thanks @yorkshire-pudding 🙏🏼 and thank you @argiepiano 🙏🏼 🙏🏼
I've given the FTB module another spin, and the UX I had concerns for has solved, but there is still a small bug for a follow-up:
But perhaps we can leave that to be solved in the PR for merging the functionality into core. Anyone has the time/energy to work on that?
Other UX/consistency things that I would like us to consider when in core:
theme_label_machine_name()
, and not be clickable.node
, current-date
, current-page
to [node:...]
, [current-date:...]
, [current-page:...]
etc. instead.That's it for now. Curious to see what people think about the above suggestions, and I'd be happy to do another round of review and make further UX suggestions once we have a PR sandbox to play with.
@klonos said:
click on a random token -> the token gets added to the place where the cursor was placed in step 1 👍🏼 click on another token -> the token gets added to the start of the text 👎🏼 ...I'd expect it to be added right after the token added in step 3
But but but... the issue was to mirror the behavior of the core browser, and this is exactly what the core browser does!
@klonos , thanks for the "laundry list" of changes. I was going to take a stab at this, but this is a rather daunting list now - it forshadows an issue that will be in the queue for years, so perhaps it's best to leave this as contrib? Or perhaps others feel that this list is not a "sine qua non"?
Anyway, besides those changes, there are a few important code-wise changes to make to the JS. The JS is very readable and clean, but unfortunately doesn't use namespacing for variables or functions - instead it stores things as global window
variables that could be accidentally overwritten, making the code less than safe. Even if this remains in contrib, this needs to be fixed. @yorkshire-pudding I'll submit another PR fixing some of those issues.
I'm a firm believer that we shouldn't let the desire for perfect stand in the way of incremental improvement (I actually think that this should be a "Backdrop principle").
With the wonderful improvements already by @argiepiano and the suggested code namespacing refinement, perhaps this should just focus on anything important that is missing from this module when compared to core, with anything else being separated for later improvement.
... I was going to take a stab at this, but this is a rather daunting list now - it forshadows an issue that will be in the queue for years ...
Apologies @argiepiano ...I didn't mean to deter you or anyone else from working on this. Please do not let my OCDs and extensive list of "ideal" get in the way - everything I listed in my last comment can be follow-ups (or I can file PRs against any PR created by others here for consideration). I simply wanted to document these points, since I took the time to do a UX review. And since we do not have any other ticket to do that, here seemed like a relevant place to do it (it'd be silly to file a follow-up for an issue that doesn't even have a PR yet).
I'm a firm believer that we shouldn't let the desire for perfect stand in the way of incremental improvement (I actually think that this should be a "Backdrop principle").
Yes! That ^^ 👍🏼
Description of the need
I think https://github.com/backdrop-contrib/fast_token_browser is an improvement over the core version. It loads faster, has a more intuitive UI, and will be easier to make accessible to use via the keyboard (maybe it does already).
Proposed solution
Put FTB into core.
Additional information
FTB loads the tokens via ajax which makes it faster when there are a lot of tokens. And it also changes the way tokens are inserted. First the person selects the token - it shows up as selected. Then they click on / select the text field. It then gets inserted. Core has a clunkier approach which requires the unintuitive approach of selecting the field first then finding the token.
Also the modal window can be resized and moved.