-
Notifications
You must be signed in to change notification settings - Fork 8
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
Rich Text Editor events #80
base: main
Are you sure you want to change the base?
Conversation
- Add `isRichTextEditor` to existing and new composer events(Composer + SlashCommand). - Add Mention and FormattedMessage events.
- Fix typos and formatting - Add `isMarkdownEnabled` and `isPlainTextMode`
schemas/SlashCommand.json
Outdated
"Spoiler", | ||
"Shrug", | ||
"TableFlip", | ||
"Unflip", | ||
"Lenny", | ||
"Plain", | ||
"Html", | ||
"UpgradeRoom", | ||
"JumpToDate", | ||
"Nick", | ||
"MyRoomNick", | ||
"RoomAvatar", | ||
"MyRoomAvatar", | ||
"MyAvatar", | ||
"Topic", | ||
"RoomName", | ||
"Invite", | ||
"Join", | ||
"Part", | ||
"Invite" | ||
"Remove", | ||
"Ban", | ||
"Unban", | ||
"Ignore", | ||
"Unignore", | ||
"Op", | ||
"Deop", | ||
"DevTools", | ||
"AddWidget", | ||
"Verify", | ||
"DiscardSession", | ||
"RemakeOlm", | ||
"Rainbow", | ||
"RainbowMe", | ||
"Help", | ||
"Whois", | ||
"Rageshake", | ||
"ToVirtual", | ||
"Query", | ||
"Msg", | ||
"HoldCall", | ||
"UnholdCall", | ||
"ConvertToDM", | ||
"ConvertToRoom", | ||
"Me", | ||
"Confetti", | ||
"Fireworks", | ||
"Hearts", | ||
"Rainfall", | ||
"Snowfall", | ||
"SpaceInvaders" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only be tracking things to answer specific product questions, what question do these answer? Also this list is unmaintainable given its infeasible to ask contributors adding commands to also need to add them in a separate repo. The existing enum entries here were to track room leave & room invite flows respectively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it would be good from a product perspective to know which commands were actually getting used and then we could refine the commands to just those. But I will defer to product on that. @jakewb-b What do you think is useful to track?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it seems sensible to me that if we are adding commands that we would track it to understand it's validity and avoid having extraneous product surface/ UX complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be useful to have this info, for the reasons David's given, although not essential. In practice I don't know how likely we are to come back and remove slash commands that see little usage, and if we do want to make that effort there are probably other qualitative ways we can make the decision.
So, I guess it depends on how difficult it is to maintain this vs the value it offers. My take is that I'd quite like it, but I'm not going to insist on it if it creates more problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could always add it for period of time to gather the data of how broadly/commonly slash commands as a feature is used and which commands users find most useful. Once we have answered those questions we could remove the tracking of the full set so it is more maintainable. WDYT @t3chguy ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we don't remove old ones on Web it could inform what is valuable to add to mobile and EX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the question btw, I should have been more explicit. I have added a section to the description. Maybe we should add that question to a pull request template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think an enum here is infeasible, there's so many apps to keep in sync. A string is more appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea that crossed my mind and I was unsure. I thought an enum might make it more obvious what differences there was across platforms and with an enum a dev could be informed at compile time what they were explicitly ignoring. But happy to go with a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with an enum a dev could be informed at compile time what they were explicitly ignoring. But happy to go with a string.
For TS at least unless all the strings matched exactly (fully lowercase) the type would have to be entirely ignored
"Snowfall", | ||
"SpaceInvaders" | ||
] | ||
"description": "The the slash command text. e.g. /me ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this make sense @t3chguy ?
So you might nee update any reports you have that use the old format(Part
/Invite
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm good point, losing historical data would not be good. Not sure what the best solution is here then.
Any update on this PR @langleyd ? Just asking since I am seeing it, there is no rush. |
editor
enum (Legacy
,RteFormatting
,RtePlain
) to existing and new composer events(Composer
+SlashCommand
).Mention
andFormattedMessage
events.isMarkdownEnabled
Questions we are trying to answer with these events