Skip to content

Comment tagging#2312

Merged
spwoodcock merged 34 commits intodevelopmentfrom
feat/comment-tagging
Apr 11, 2025
Merged

Comment tagging#2312
spwoodcock merged 34 commits intodevelopmentfrom
feat/comment-tagging

Conversation

@NSUWAL123
Copy link
Copy Markdown
Contributor

@NSUWAL123 NSUWAL123 commented Mar 31, 2025

What type of PR is this? (check all applicable)

  • 🍕 Feature
  • 🐛 Bug Fix
  • 📝 Documentation
  • 🧑‍💻 Refactor
  • ✅ Test
  • 🤖 Build or CI
  • ❓ Other (please specify)

Related Issue

Describe this PR

  • User mentions functionality in Update Review State modal, & display comment via popup if the user is mentioned on the comment in mapper frontend

Screenshots

Uploading Screen Recording 2025-04-09 120527.mp4…

@github-actions github-actions Bot added enhancement New feature or request frontend Related to the frontend code labels Mar 31, 2025
@NSUWAL123 NSUWAL123 changed the title omment tagging Comment tagging Mar 31, 2025
@spwoodcock
Copy link
Copy Markdown
Member

Looks good!

spwoodcock and others added 9 commits April 8, 2025 15:37
…refilled) (#2347)

* hide feature selection in web forms

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
…ads (#2349)

* cache form xml and ODK Web Forms js library

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
…f URLs pt1 (#2348)

* feat: WIP add osm-fieldwork api for form attachment download + API endpoint

* refactor: hardcode values until PR continued
@github-actions github-actions Bot added docs Improvements or additions to documentation backend Related to backend code devops Related to deployment or configuration dependency:osm-fieldwork Requires updates in osm-fieldwork labels Apr 8, 2025
@NSUWAL123 NSUWAL123 marked this pull request as ready for review April 9, 2025 06:48
@NSUWAL123 NSUWAL123 requested a review from spwoodcock April 9, 2025 06:48
Copy link
Copy Markdown
Member

@spwoodcock spwoodcock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty cool 🙌

The video failed to upload tho

import { useAppDispatch, useAppSelector } from '@/types/reduxTypes';
import { task_event } from '@/types/enums';
import { featureType } from '@/store/types/ISubmissions';
import '@/styles/rc-mentions.css';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theres no convention for this, but I always place CSS imports at the top of files to groups them / make it obvious

Comment thread src/frontend/package.json
"pako": "^2.1.0",
"pmtiles": "^3.0.6",
"qrcode-generator": "^1.4.4",
"rc-mentions": "^2.19.1",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more react specific components we add, the more we will need to refactor in future.

I dont see the future of the management frontend being in React (in the long run).

As much as possible, could we try to use web components instead?

For example, what about this text expander web component from Github?
https://github.com/github/text-expander-element

It also picks up a specific tag (e.g. @ or #) from a text input, with a hook to do something.

I would suggest we only query the backend after 3 characters or more are typed. Hopefully this narrows it down a fair bit before searching the db

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I'll have a look at text-expander-element.
Also completely agree with you on querying after 3 characters

SetUserListForSelectLoading: (state, action: PayloadAction<boolean>) => {
state.userListLoading = action.payload;
},
SetUserNames: (state, action: PayloadAction<UserStateTypes['userNames']>) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will set all available usernames from the backend into the browser right?

If we have 10,000 users at some point, do you think this would cause an issue?

No need to over-optimise at the start (we dont have 10,000 users...), but its always good to keep scalability in mind when designing solutions 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So currently instead of showing all user list, I am querying if there's 1 character, but will change to 3 min character as you mentioned!

@spwoodcock
Copy link
Copy Markdown
Member

As a tradeoff for performance and scalability, we could consider the following:

  • On first 3+ character input (e.g. "@chr"), query the API.
  • Store the result (["chris", "christina", "chrissy"]) in the store (redux or svelte).
  • As the user continues to type (e.g. "@Chri"), filter the local store list.
  • Then finally, if the user selects an option, or deletes characters (< 3 chars), or deletes the @, then we clear the local store values.

@spwoodcock
Copy link
Copy Markdown
Member

spwoodcock commented Apr 9, 2025

The web component I linked provides a demo implementation:
https://github.com/github/text-expander-element/blob/main/examples/index.html
https://github.github.io/text-expander-element/examples/

    <text-expander keys="@">
      <textarea autofocus rows="10" cols="40"></textarea>
    </text-expander>

        expander.addEventListener('text-expander-change', event => {
          const {key, provide, text} = event.detail
          if (key === '@') {
            const menu = document.createElement('ul')
            menu.classList.add('menu')
            menu.role = 'listbox'
            // TODO instead of hardcoding values, we would query the API if 3 characters present
            for (const issue of [
              '#1 Implement a text-expander element',
              '#2 Implement multi word option',
              '#3 Fix tpoy',
              '#4 Implement #12',
              '#5 Implement #123 and #456',
            ]) {
              if (issue.toLowerCase().includes(text.toLowerCase())) {
                const item = document.createElement('li')
                item.setAttribute('role', 'option')
                item.textContent = issue
                item.setAttribute('data-value', issue.split(' ')[0])
                item.id = `option-${issue}`
                menu.append(item)
              }
            }
            provide(Promise.resolve({matched: true, fragment: menu}))
          }

@NSUWAL123
Copy link
Copy Markdown
Contributor Author

@spwoodcock tried implementing text-expander-element but doesn't seem to work.

  1. When using it with redux-store, the list doesn't update (the states are one state behind) due to the list populating after the change event trigger & list being built inside the change event.
  2. Also when directly fetching the list under the change event, the list can be consoled but doesn't render. It's very weird as the static list would render.
    Also tried many other ways but doesn't work 😵‍💫

@spwoodcock
Copy link
Copy Markdown
Member

No worries @NSUWAL123 - it was maybe a bit ambitious to use that web component, as I haven't tested it myself!

It's not a huge deal, as the React frontend doesn't really use any web-components anyway - updating most things to use them would be a big undertaking.

The key thing is to ensure we are using web components everywhere possible in the mapper frontend.

Thanks for trying though!! 🙏

@spwoodcock spwoodcock merged commit 7b37936 into development Apr 11, 2025
6 checks passed
@spwoodcock spwoodcock deleted the feat/comment-tagging branch April 11, 2025 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend Related to backend code dependency:osm-fieldwork Requires updates in osm-fieldwork devops Related to deployment or configuration docs Improvements or additions to documentation enhancement New feature or request frontend Related to the frontend code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants