Fix hidden description on mobile

[thombruce]

Closes #77

By submitting this pull request, you agree to follow our Code of Conduct: https://github.com/thombruce/.github/blob/main/CODE_OF_CONDUCT.md

---

Internal use. Do not delete.

• [x] Tests passing
• [x] Coverage sufficient
• [x] Manual review
• [x] No A11y regression
• [x] Translations provided or not needed

[netlify[bot]]

✅ Deploy Preview for toodles ready!

Name	Link
🔨 Latest commit	add6f25d652f8d38063d6bbbeeefd265f582e7ac
🔍 Latest deploy log	https://app.netlify.com/sites/toodles/deploys/64aad64f1389d20008f95370
😎 Deploy Preview	https://deploy-preview-105--toodles.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[github-actions[bot]]

Coverage Summary for `./packages/web`

Status	Category	Percentage	Covered / Total
🟢	Lines	67.52% / 60%	420 / 622
🟢	Statements	67.52% / 60%	420 / 622
🟢	Functions	60.97% / 60%	25 / 41
🟢	Branches	69.84% / 60%	44 / 63

File Coverage

File	Stmts	% Branch	% Funcs	% Lines	Uncovered Lines
packages/web/src/components/ContextTag.vue	20%	100%	0%	20%	4-15
packages/web/src/components/HashTag.vue	20%	100%	0%	20%	4-15
packages/web/src/components/ProgressBar.vue	70.17%	66.66%	100%	70.17%	16-23, 26-34
packages/web/src/components/ProjectTag.vue	20%	100%	0%	20%	4-15
packages/web/src/components/TagTag.vue	71.42%	100%	0%	71.42%	6-7
packages/web/src/components/TodoItem.vue	80.7%	40%	28.57%	80.7%	21, 26-27, 33-40
packages/web/src/components/TodoList.vue	73.68%	100%	100%	73.68%	11-15
packages/web/src/components/TodoPriority.vue	20%	100%	0%	20%	4-15
packages/web/src/components/TodoText.vue	61.7%	60%	85.71%	61.7%	15-18, 21-25, 30, 33-36, 39-42
packages/web/src/models/Todo.ts	89.65%	76.92%	100%	89.65%	35-41, 55-56
packages/web/src/plugins/dexie.ts	55.12%	100%	66.66%	55.12%	38-41, 45-55, 59-78
packages/web/src/plugins/lunr.ts	78.57%	100%	0%	78.57%	10-12
packages/web/src/plugins/timepiece.ts	71.42%	60%	50%	71.42%	14-19
packages/web/src/stores/todos.ts	72.57%	90%	83.33%	72.57%	21, 25, 29-31, 35-37, 41-43, 47-49, 53-55, 59-61, 65-67, 71-73, 77-79, 83-85, 89-91, 95-97, 103, 107-115

[thombruce]

For some reason, I can't add a todo on mobile...

Neither the return key nor form button are working. This is also true on the production site.

It's a new regression, clearly, but not one introduced here.

This does hinder my testing capabilities for this at this time.

[thombruce]

use value of var for MultiBar actions did not fix the newly noticed issue, but does not regress the functioning of the app in any other way (it's the more canonical way to handle form bindings anyway).

So... what's the problem?

Since text.value is not cleared, issue must exist either at or before store.addTodo(text.value). And since what comes before this is a pretty straightforward validation... likelihood is there's an issue in addTodo in the store module.

[thombruce]

For reference, here's addTodo:

  function addTodo(editable: string) {
    const todo = new Todo(editable)
    list.value.push(todo)
    // db sync:
    db.todos.add(todo).then().catch()
  }

It accepts a string, instantiates a new Todo with this string, pushes the new Todo into the Pinia list of todos (at which point it should appear in the UI), then it adds that Todo to the persistence store.

That all looks good to me.

Tempted to get rid of the validation to see if it's standing in the way.

[thombruce]

The issue could be with the Todo model at models/Todo.ts.

Experiment by removing setTags (which by the way is run twice on instantiation - we should move the one in the constructor inside of the else condition).

If that doesn't work, we could experiment with the simplification of handling the editable value - we used to save the raw description, worth a shot.

And if that doesn't work... I'm out of ideas but I'm probably missing something really simple.

[thombruce]

Good news: It is setTags at fault - removing it resolves the issue.

Bad news: We definitely still haven't fixed the hidden description issue - ...but we probably just need to explicitly state element widths higher in the hierarchy.

Okay... two things to fix then.

[thombruce]

Changes reverted to the point where I think it worked, so that I can make more progress elsewhere.

It isn't a single tag at issue, but apparently the entire setTags function, so I'm simply skipping it during the instantiation and updating of todos for now.

What's the issue? Something simple probably like scratch everything I've said here, what are those commas doing there and how long have they been there?

[thombruce]

Yeah, it wasn't the commas; they're just a reminder that I should have a linter installed.

I'm reverting to having no use of setTags(). The issue is in that function somewhere, I just don't see what it is. Maybe in one of the regexes, maybe in how the function is structured, I do not know. But it's in there somewhere.

[thombruce]

I am now able to add todos on mobile, but I still cannot read them. Let's make them readable, and then we'll circle back to the setTags issue.

[thombruce]

It looks to me like...

ul -> li does not have 100% width in my mobile browser, but does in all other conditions.

Lists should apparently have 100% width by default, but maybe this is changed by Tailwind or by the mobile Chrome browser.

I intend to focus on a CSS refactor in future (because I am terrible at CSS), so no worries about sloppy implementations at this time... Do we just go ahead and add .w-full to the ul and li that contains each TodoItem?

We could alternatively replace each li with a div and enclose everything in a div instead of ul. We are working against the default stylings of ul and li, after all. Tailwind gives us an assist there, by stripping some of their default stylings... but maybe we shouldn't be using them at all.

[thombruce]

Replacement of ul -> li with full width div does not resolve issue. I've also experimented with removing the TodoText and floating the trash icon right, in theory demonstrating that actually the div does not occupy the full width.

This is a large-scale structural issue, then, requiring some refactoring of the CSS throughout.

Even then... there remains some chance that the TodoText still simply is not showing on mobile. This could be because of regexes in TodoText that are only present there (and share similarity perhaps with those in setTags - these issues could relate after all). It is certainly possible that these two places have some shared regex pattern or utilisation of them that causes their failure specifically on my old iPhone.

[thombruce]

That's it exactly, I think. The iOS version on my iPhone is 15.7.7, which does not support lookbehind in regular expressions:

[thombruce]

Either...

1. we mimic lookbehind some other way
2. we explicitly drop support for devices older than the iPhone 8 (first to support iOS 16.4)

With 86% of users having support for RegEx lookbehinds... the error on my device is the exception, not the rule. But 14% is still a large group of users... and I do happen to be one of them, and would like to use Toodles from my phone.

I'll consider workarounds.

[thombruce]

Since we're wanting to check for but omit the space and character preceding the tag (for project-like tags, e.g. +project), we can instead of a lookbehind use a capturing group around the part that actually interests us. For example:

"This has a +project or +two in it".match(/(?:^|\s)\+(\S+)/g)

Unfortunately .match() returns the full matches, space and all (so ' +project' and ' +two' for the above).

We'll have to look at other options, of which there are a few. But using the (capturing group) is a solid plan. Should work just the same... just about.

[thombruce]

[..."This has a +project or +two in it".matchAll(/(?:^|\s)\+(\S+)/g)]

This yields an array where each item in the array is itself an array like...

[' +project', 'project', index: 10, input: 'This has a +project or +two in it', groups: undefined]
[' +two', 'two', index: 22, input: 'This has a +project or +two in it', groups: undefined]

So to access the results we want for a specific instance it would be instance[1].

This works in 93.8% of browsers (around 95% of tracked browsers) including iOS 13+.

And it... does exactly the same as the results we were aiming for from the lookbehind. This is a working solution.

[thombruce]

Ah, in TodoText.vue we're using it in a .split():

[priority, ...description.split(/((?<=^|\s)(?:\+|@|#|[^\s:]+?:)\S+)/g)]

For one, I'm pretty sure we don't need to split on start of string (that's implied). [EDIT: Actually, it's part of the pattern for matched items.] So it's whitespace, followed by a +, @, # or as few non-whitespace, non-colon characters as possible followed by a :, followed by any number of non-whitespace characters.

And yes, identifying the whitespace before the tag is important as it's the difference between... a @context and an email@address.

We could still use .matchAll() and split on the indexes returned? That might work. .matchAll() and map the indexes, split a string at those indexes using... well, maybe we don't map - maybe we iterate over the iterator matchAll provides and slice and push into an array at each index along the way (not forgetting to push the final segment too).

A better .split() pattern would be more ideal, but yeah... that would probably also work too - more complicated but it would work. So what other split patterns might work.

[thombruce]

There might be a critical point in the complexity of an iteration approach where RegExp.exec() makes more sense than String.matchAll(), so let's keep that in mind. It also has wider support. Though the syntax will not be as nice, I understand...

[thombruce]

In any case, we're introducing a level of complexity that justifies a submodule. Call it regexes.ts or patterns.ts and maybe it lives in the plugins folder. Both TodoText.vue and Todo.ts could import it or import their essential regex patterns and functions from there. It seems reasonable to group them together, particularly as regular expressions are a specific kind of skill (a lot like i18n) and I think this alone justifies their having an isolated space where skilled regex writers/editors can propose changes to them.

I do think I also need to write a function and I really don't want the TodoText component to be too cluttered by that. That's the main reason I'm proposing this, if we're being honest. But I'll stand by the words above too!

[thombruce]

Lookbehinds are also used in stores/todos.ts, for example:

const filtered = todos.filter(t => new RegExp(`(?<=^|\\s)\\${project}(?=\\s|$)`).test(t.description))

These functions predominantly concern lookups for specific pages, (e.g. the project route at /+project).

In this case, they're just test expressions. It doesn't really matter that a lookbehind is used at all, so long as the text it's looking for is present. Easy conversion to a non-capturing group.

[thombruce]

Compare:

return [priority, ...description.split(/((?:^|\s)(?:\+|@|#|[^\s:]+?:)\S+)/g)]
return [priority, ...description.split(/(?:^|\s)((?:\+|@|#|[^\s:]+?:)\S+)/g)]

The first converts the lookbehind to a non-capturing group, but since it is still included in the surrounding capturing group, the space is included in the tag side of the split: " +project"

The second shifts the start of the capturing group to after the non-capturing group. This ends up omitting the spaces entirely.

---

If we took the second approach, we'd need to reinsert the spaces. If we took the first, we'd need to adjust subsequent matching logic to account for the potential presence of spaces at the start of the string (start of string is also a valid capture).

With a small modification to subsequent regexes, the first approach kind of already works for everything except custom:tags. The only reason custom tags don't work is because they have a background colour which reveals the presence of the space:

I'd still prefer the styling to be applied only to the non-whitespace part (i.e. the actual tag), but this could be a working approach...

[thombruce]

We could split the string entirely on spaces while ensuring spaces are still captured, e.g.

"This is a +project @context #hashtag and custom:tag @test".split(/(\s)/g)
// ['This', ' ', 'is', ' ', 'a', ' ', '+project', ' ', '@context', ' ', '#hashtag', ' ', 'and', ' ', 'custom:tag', ' ', '@test']

This would mean more strings are then subsequently being tested, which means a performance hit (a potentially significant one for long todos or large collections of todos). Each string goes through five matching attempts after this, aiming to determine if it is a priority, a project, a context, a hashtag or a tag.

Priority technically only needs to be checked for the first item. So we can make this change:

- TodoPriority(v-if="priorityMatcher(item)" :priority="item" @input="update")
+ TodoPriority(v-if="item === items[0] && priorityMatcher(item)" :priority="item" @input="update")

...though if we're splitting on spaces instead now, this could match identical strings later in the text. Better to introduce the index to the for loop and check that instead.

We could also introduce a universal tag check that just checks to see if a string matches any tag pattern. This would reduce the four checks to one unless matched. If matched, we'd do subsequent checks to determine the kind.

We could also skip checks determined by whether or not the Todo actually has such tags (we do store them separately now, and fixed this behaviour for older browsers earlier in this PR).

---

Here's another idea...

Could we at the point that we setTags() in the model (now fixed), also store the indexes of the tags?

We do get the indexes as part of the .matchAll() function, but the index is off by one if it's preceded by a space. However, this is easily checked: if i[0] !== i[1] it is because i[0] is preceded by a space; i[1] is the capturing group. So if they don't match, add one to the index. If they do match, don't.

How does this help us? Can we say, "split the text at multiple indexes". .slice() can give us part of a string determined by a start and end index, but we want the whole string separated into an array at N indexes.

I think this is a good idea, I just don't see how it works without introducing a lot more complexity.

[thombruce]

Yeah, I really don't love the idea of splitting on spaces and having so many more strings to test. Even with improvements to efficiency, wouldn't those efficiency improvements better serve a smaller set of strings? Genuinely unsure; there's a lot of complexity here and I'm not yet benchmarking any of it.

Still prefer to separate on tag patterns.

Presently thinking that perhaps the .split() approach that also captures leading spaces as part of the tag is preferable. We can then subsequently treat those tags before passing them to the tag component that styles them.

[thombruce]

Learned something about how .split() works and I think this approach does the trick:

return [priority, ...description.split(/(^|\s)((?:\+|@|#|[^\s:]+?:)\S+)/g)]

Rather than a lookbehind or a non-capturing group, we make ^|\s a capturing group. So if the pattern is matched, we get two captures pushed into our array; the space and the part we're interested in. But if there are no matches... it isn't separated on spaces.

Yes, this results in some superfluous spaces, but it does not separate on every space as was a suggested approach above. This results in slightly more for subsequent tests, but not as much as we would have if we separated on spaces.

[thombruce]

If the text starts with a tag, we also do get two empty strings at the start (e.g. ['', '', '@home', ...]).

Why? Because we're splitting on ^ (start of string). Can we omit and/or move it elsewhere? Or...

Just add .filter(n => n) to the end to remove empty elements. That'll do it.

[thombruce]

In future, either a linter might catch use of lookbehinds or we might consider testing against older versions of... well, it would have to be Node, right? Not ideal, since we're targeting browsers and other behaviours may break in old Node environments, but that's an option.

Ideally, we could test in a browser-like environment - so like webkit? - and ensure that things behave properly for specific versions of the environment, aiming for ~95% support (even higher would be better).

We should look into the best ways to ensure that. But this... This is done.