The diff extends navInfo object with new chat-mode - which is an enum - 'view' or 'create', and`selectedUserList` containing list of users read from URL.
The aim is to keep URL while creating (or finding existing) thread look like /chat/thread/new/<list_of_added_users_separated_by_comma>.
So for example /chat/thread/new/1,7,48/ is going to redirect user to chat creation screen with selected three users with IDs 1,7 and 48.
Empty list will create URL look like /chat/thread/new/
Details
Details
The change doesn't affect current navigation. New paths are not used in application yet. I tested the navigation after introducing more thread creation logic (in next diffs)
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- jacek/tmp
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Can you provide some more context for why we need a new tab type for this? It seems pretty confusing to me... in the UI, we have just three tabs. It would seem like chat creation should be part of the chat tab?
Comment Actions
Yes, it's in fact confusing... The initial idea (before my exam break) was to support new app state - chat creation - with separate "tab", but as you notice, we will not create different tab in UI for it. The drawback is also that we lose the chat-creation state when switching between apps.
After discussion with @palys-swm, we concluded, it would be the best to support navigation state by introducing a separate field - chatMode in navInfo - and keep chat state in a way similar to settingsSection. We also agreed, that as a follow-up task it would be good to refactor the navigation to keep navigation state in more nested structure - so all chat-related fields (activeChatThreadID, pendingThread, selectedUserList and chatMode) would be put inside the e.g. ChatNavigationState object in navInfo.
So, I'm going to update this diff (and it's name) to replace the new tab with a separate field in navInfo.
Comment Actions
Agree with both of those conclusions!
We also agreed, that as a follow-up task it would be good to refactor the navigation to keep navigation state in more nested structure
We could create a new backlog task for this one.
| web/url-utils.js | ||
|---|---|---|
| 59–62 ↗ | (On Diff #14391) | Can simplify this with a ternary. |
| lib/utils/url-utils.js | ||
|---|---|---|
| 35 ↗ | (On Diff #14391) | What is the reason behind using commas in this url? We already have getLocallyUniqueThreadID where we use plus signs instead - can we do the same here to keep it consistent? |
| web/url-utils.js | ||
| 126–131 ↗ | (On Diff #14391) | We're modifying the newNavInfo so there's no benefit in destructuring it. We can also modify the urlInfo.settings code to use the same pattern |
| lib/utils/url-utils.js | ||
|---|---|---|
| 35 ↗ | (On Diff #14391) | I think there is no special reason for that and it could be + signs as well. When we were discussing the solution in June I think we agreed to ,, but I can easily change it to + to make it more consistent with other URLs. |
| web/url-utils.js | ||
| 126–131 ↗ | (On Diff #14391) | Thanks for catching |
Comment Actions
Looks good.
(Adding unit tests for infoFromURL(...) seems like it could be a quick thing?)
| web/url-utils.js | ||
|---|---|---|
| 59 ↗ | (On Diff #14715) | We could be more explicit here with users.length > 0 instead of relying on truthiness but don't feel strongly, up to you |
| 59 ↗ | (On Diff #14715) | We could maybe rename this to something like potentiallyTrailingSlash so it still makes sense when it's set to the empty string? |
| 117–120 ↗ | (On Diff #14715) | could maybe do const chatMode = urlInfo.threadCreation ? 'create' : 'view'; or something |
| lib/utils/url-utils.js | ||
|---|---|---|
| 37 ↗ | (On Diff #14831) | Found out that wrapping + into square brackets [+] could be used instead of double escape: \\+ |