Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add failing test for custom regex that contains trigger #299

Closed
wants to merge 2 commits into from

Conversation

mxstbr
Copy link

@mxstbr mxstbr commented Jan 28, 2019

What did you change (functionally and technically)?

I want to add mentions to my input which show @max and also send @max to the server. I thought the custom regexp feature introduced in #255 would help me achieve that, but it only seems to work if the custom regex does not include the trigger.

I would love for this to work, any ideas what the necessary change would be?

I want to add mentions to my input which show `@max` and also send `@max` to the server. I thought the custom regexp feature introduced in signavio#255 would help me achieve that, but it only seems to work if the custom regex does not include the trigger.
mxstbr pushed a commit to withspectrum/spectrum that referenced this pull request Jan 28, 2019
Ideally, we would fix mentions by setting `markup="@__display__"` for
the `MentionInput`. Unfortunately due to a bug in react-mentions, this
doesn't work: signavio/react-mentions#299

This works around the issue by inserting `@[username]` into the value,
and then removing the brackets before sending to the server!

Closes #4587
@jfschwarz
Copy link
Contributor

jfschwarz commented Jan 28, 2019

The problem is not related to the custom regexp. You seem to confuse the display and id attributes/placeholders. It's important to understand that the data prop passed to the <Mention/> element is used exclusively when the user hits the trigger char and selects a mention.

Parsing the markup value and turning it into the readable plain text value happens without reading from data. You could use the displayTransform in a different way to fix your test:

<MentionsInput
    value="@one and @two"
    markup="@__id__"
    regex={/@(\S+)/g}
    displayTransform={id => data.find(d => d.id === id).display}
>
    <Mention trigger="@" data={data} />
</MentionsInput>

@mxstbr
Copy link
Author

mxstbr commented Jan 28, 2019

In our specific case, id and display are equal. Our data looks like this:

[{
  id: 'brian',
  display: 'brian'
}, {
  id: 'bryn',
  display: 'bryn'
}]

For some reason, the setup in the tests breaks when used in our app. Do you want me to create an isolated reproduction instead of this test?

@jfschwarz
Copy link
Contributor

Do you want me to create an isolated reproduction instead of this test?

Sure. This would be helpful.

@mxstbr
Copy link
Author

mxstbr commented Jan 28, 2019

The latest commit changes the test case to better reflect our production setup. Does that help narrow down the possibilities?

@mxstbr
Copy link
Author

mxstbr commented Jan 28, 2019

You can also see the actual, production code and workaround in withspectrum/spectrum#4588, although I doubt it is helpful without understanding the whole app 😅

@jfschwarz
Copy link
Contributor

Your adjusted test does not fail anymore ;)

@mxstbr
Copy link
Author

mxstbr commented Jan 28, 2019

Okay, here is an isolated reproduction: https://codesandbox.io/s/yk48y80m2x

Note how the suggestions disappear after typing the first character. If I change the markup option to @[__id__] this does not happen anymore.

What am I doing wrong?

@jfschwarz
Copy link
Contributor

Thank you, the repro helped. I got the problem now.

We apply the string matching for the trigger character only on the substring between the caret position and the end of the previous mention. In your case, as soon as the user types the first character after the @ we have a valid mention in the value, so the trigger char string matching then only starts after that markup of that mention. (see: MentionsInput.js#L619 / MentionsInput.js#L631)

It could be an option to simply apply the trigger regexp onto the entire substring from the start of the value to the caret position. 🤔

@mxstbr
Copy link
Author

mxstbr commented Jan 28, 2019

I am not sure I follow entirely as I do not know the codebase at all, so whatever you say is the right idea I can look at implementing if you don't get around to it! 👍

@jfschwarz
Copy link
Contributor

On second thought, using the same value for markup and display purposes comes with some problems (even with the solution I had in mind):

It means, that while typing characters after the @ to filter the suggestions this word would already be treated as a complete mention. Thus, pressing backspace would remove the entire word instead of only the last character of the search string.

The component basically works as a pure function of value and caret position. I'd like to avoid introducing a state for tracking unconfirmed mentions as this would introduce significant complexity. Without this state, there would be no way to distinguish whether we're looking at an unterminated search or an actual, user-confirmed mention. With your current workaround, you effectively have a "lock-in" of mentions on submit, which might actually be the best approach.

Your workaround might still turn unconfirmed mentions into confirmed ones, when we map a value of "@[brian] and @b" to "@brian and @b" on submit and then back to "@[brian] and @[b]". (This could be mitigated by checking if the IDs are valid inside the mapping function.)

akpx added a commit to akpx/spectrum that referenced this pull request Feb 4, 2019
* update background-jobs documentation

* Fix chatInput focussing

* Fix sending plaintext messages

* Remove unnecessary double-read of attached media

* Resolve React warnings

* Fix creating new DM thread

* Revert "Fix creating new DM thread"

This reverts commit d8e9cc8.

* Fix flow

* Update background-jobs.md

* Don't include the req path in the datadog key

This is an anti-pattern, as it'll blow up our DataDog storage costs.
Instead, we should be using a logging service like Splunk to dig into
specific slow requests after noticing abnormalities in the
metrics. Will tackle that next most likely, but this should be a good start!

Thanks to the folks in #observability for pointing that out!

* Dont track connection pool size in DataDog, track query response times and sizes

* Properly flow-type statsd

* Add statsd logging in dev

* Log instance hostname

* Upgrade apollo-cache-inmemory

* Better network disabled styling on input

* Fix styling if quote-replying and uploading media at same time

* Dont use alpha version of new apollo inmemory

* Add codesandbox to explore

* Add codesandbox url regex for better embeds

* Add packages for deploys

* Add the rest

* Update react-modal to version 3.8.1

* Update electron-builder to version 20.38.4

* Focus chatinput on quote message click

* Fix media messages arriving after text messages

* Handle websocket connection states

* Track job queue metrics

* Update react-transition-group to version 2.5.2

* Eliminate warning in console due to incorrect html element nesting

* Update debug to version 4.1.1

* Update debug to version 4.1.1

* Update debug to version 4.1.1

* Update debug to version 4.1.1

* Update debug to version 4.1.1

* Update debug to version 4.1.1

* Update debug to version 4.1.1

* Update validator to version 10.10.0

* Update serialize-javascript to version 1.6.1

* Remove use of Google+ APIs

* Downrank watercooler threads in digests

* Nicer reputation string in digest emails

* Add function to check if localStorage is allowed

* Trigger login modal in the joinChannel upsell if no currentUser

* Remove localStorage check

* Make logged out users sign in before seeing chat input

* Remove unused timeInUTC variable

* Fix e2e tests

* Better string, fix test snapshots

* Update rimraf to version 2.6.3

* Update aws-sdk to version 2.383.0

* Update aws-sdk to version 2.383.0

* Update aws-sdk to version 2.383.0

* Update aws-sdk to version 2.383.0

* Update aws-sdk to version 2.383.0

* Add missing username property in onValidationResult object

`onValidationResult` function takes an object with prop error, success
and username.

* Make username optional

* Fix flow

* Remove unused variables from chatInput

* Explicitly target chat input from e2e tests

* Move data-cy="chat-input" to actual input

* Use .clear() instead of .type() in e2e tests

* Remove .only in thread_spec, fix /new/thread tests

* Upgrade to draft-js-import-markdown@latest to fix code blocks

* Make plaintext message editing work

* edit in plaintext on the frontend

* Fix flow

* Fix editing e2e test

* Match input font weight to rendered message font weight

* Remove invalid dataCy dom node property

* Change editor to allow multiline messages

* Remove empty line while editing

* Fix incorrect focussing on chat input while editing message

* Make single line breaks work!

* Upgrade to [email protected]

This implements query parsing and validation caching, reference
apollographql/apollo-server#2111, and should
thusly provide a nice speedup for our frontend's queries!

* add delete cover feature to edit community form

* add coverphoto flow type

* Add extra checks at send time for valid user recipient

* Async await

* Remove unnecessary code

The code block for the `SET_SEARCH_VALUE` type and the `SET_SEARCH_VALUE_FOR_SERVER
` type are indentical. And, the `SET_SEARCH_VALUE` type isnt used in any action in the codebase.

* Better keycode management

* Enable flow in keycodes file

* Fix flow

* Focus the edit message input when edit message button is clicked

* Move keycodes.js to src/helpers

* Update source-map-support to version 0.5.10

* Update view error message

* Fenced code blocks while editing

* WIP: Implement GraphQL Rate Limiting

* Make graphql-rate-limit work!

* Show rate limit errors to end users

* Adjust createChannel/createCommunity rate limits

* Make flow happy

* Use the job queue redis instance for rate limit data

* Log rate limit errors to sentry in prod

* Update to new version of gql-rate-limit, use string syntax

* Make flow happy

* Add flow stub for request-ip

* Update react-transition-group to version 2.5.3

* Remove draftjs block type validation in messages

This is confusing now that we have the plaintext input, and doesn't add
anything as invalid blocks are treated as text blocks anyway on the
frontend.

* Disable rate limiting unless in prod

* Gender-neutral rate limit error message, better addMessage rate limit

* Update electron-builder to version 20.38.5

* Support hyperlinks in messages

* Make flow happy

* Update validator to version 10.11.0

* WIP: Add mention suggestions to chat input

* add delete cover feature to createCommunityForm

* Update validate-draft-js-input.js

* Stop submitting message during IME is composing

* Fix oopsie

* Search users on type

* It works!

* Fix flow

* Cleanup unnecessary code changes

* Fix mentionsuggestions props

* Fix dm chatinput onfocus and onblur handlers

* Fix typo

* Upgrade graphql-rate-limit

This fixes a critical bug that is preventing some folks from posting
messages

* Update bull to version 3.5.3

* Update bull to version 3.5.3

* Update bull to version 3.5.3

* Update bull to version 3.5.3

* Update bull to version 3.6.0

* Update bull to version 3.6.0

* Update bull to version 3.6.0

* Update bull to version 3.6.0

* Hotfix Android thread creation

* Fix hyperion memory leak

Closes #4573

* Polish styles and prepopulate suggestions box

* Update moment to version 2.24.0

* Update immutability-helper to version 2.9.1

* Show participants and search results in mention suggestions

1. Enter `@`
2. Participants are shown
3. Enter `@b`
4. Participants filtered and sorted by whether they have a `b` in them are shown
5. In the background, we do an API req
6. If there are users that come back from the API, we append them to the list of suggestions

This is a much nicer experience than hiding the participants if you just want to filter them

* Fix bug with null participant

* Update graphql-tools to version 4.0.4

* Fix login redirects

* Only show 10 suggestions, make sure suggestions have usernames

* Pass participants to chatinput for SSR views

* Darker shadow for contrast

* Return original participants if user removes search query

* If switched thread has no messages, remove participant suggestions

* Ensure author is a suggested participant

* Better styling, fewer suggestion results

* Improves suggestion sort to factor in name

* Fix capitalization for rendering and filtering

* Fix flow

* Ensure that local results will appear even if no remote results exist

* Fix the default sort if search query isn't at the 0 index

* Properly transform author username suggestion

* Refactor to only perform suggestion transformations in one place

* Provide transformation function in DM sorting

* Fix flow

* Workaround react-mentions bug with markup option

Ideally, we would fix mentions by setting `markup="@__display__"` for
the `MentionInput`. Unfortunately due to a bug in react-mentions, this
doesn't work: signavio/react-mentions#299

This works around the issue by inserting `@[username]` into the value,
and then removing the brackets before sending to the server!

Closes #4587

* Add <MentionsInput /> component

* Fix dataCy passthrough to mentionsinput

* Update graphql-cost-analysis to version 1.0.3

* Update draft-js-export-markdown to fix escaped code chars

* Update draft-js-import-markdown to fix link swallowing

* Update draft-js-import-markdown to version 1.3.0

* Update highlight.js to version 9.14.1

* Update graphql-rate-limit to version 1.2.3

* Update aws-sdk to version 2.395.0

* Update aws-sdk to version 2.395.0

* Update aws-sdk to version 2.395.0

* Update slate to version 0.44.10
@jfschwarz jfschwarz closed this May 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants