feat(Text): add "me" option to atMention prop#277
Conversation
codepretty
requested review from
dvdzkwsk,
kuzhelov,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #277 +/- ##
======================================
Coverage 91.8% 91.8%
======================================
Files 63 63
Lines 1171 1171
Branches 173 152 -21
======================================
Hits 1075 1075
Misses 92 92
Partials 4 4
Continue to review full report at Codecov.
|
|
it seems that at this point we should really be concerned about Teams-domain aspects propagating into core Stardust API. Probably, this is the right point to introduce wrapper for the |
src/components/Text/Text.tsx
Outdated
| export interface ITextProps { | ||
| as?: any | ||
| atMention?: boolean | ||
| atMention?: 'me' | 'other' |
There was a problem hiding this comment.
I would think that we would still support the boolean case here. Some themes (like GitHub) don't have the concept of me vs other. In those cases, themes have the need of styling based on the presence of this prop alone (boolean).
<Text atMention>@someone</Text>
<Text atMention="me">@me</Text>
<Text atMention="other">@other</Text>What these atMention values look like or whether or not they look different between their usages is up to the theme.
There was a problem hiding this comment.
Actually, thinking through #277 (comment) more carefully I think we only need boolean and me, correct? An atMention is a generic mention while atMention="me" is the only specifier needed for custom formatting.
src/components/Text/Text.tsx
Outdated
|
|
||
| /** Set as @mention Text component */ | ||
| atMention: PropTypes.bool, | ||
| /** An @mention Text component can be formatted for myself mentioned or an other person mentioned */ |
There was a problem hiding this comment.
Phrasing is a bit odd here. There is no "@mention Text component". There is a "Text component" that can be used to show an "at mention". One potential rewrite here might be:
/**
* At mentions can be formatted to draw users' attention.
* Mentions for "me" can be formatted to appear differently.
*/| ...(truncated && truncateStyle), | ||
| ...(atMention && { color: v.atMentionTextColor }), | ||
| ...(atMention && | ||
| atMention === 'other' && { |
There was a problem hiding this comment.
I would think that we would want to also handle the boolean case here as the "default" at mention formatting. The exception to the atMention usage would be mentions to me. So my proposal would be the majority case (just a boolean prop) would use the majority styling (other).
atMention === true || atMention === 'other' && ...| color: v.atMentionOtherTextColor, | ||
| }), | ||
| ...(atMention && | ||
| atMention === 'me' && { |
There was a problem hiding this comment.
No need to check for atMention && first. If there is no atMention then the next logical check atMention === 'me' will fail. It is sufficient to only have this one check.
|
Updating everyone so we're on the same page:
Response: In Bellevue we initially shared these same thoughts. However, after considering the "mention" usage in other chat applications the concept of "my mentions" vs "regular mentions" does exist. Since we already have the Conclusion: At the time of writing, both Prague and Bellevue have been updated on this and we're on the same page as far as accepting these Text changes for now. Forward thinking: There is probably a refactor warranted to the Text component at some future time as it is currently taking on a lot of responsibility. It is needs to have props for various usages (important, etc) as well as props for utility (fontWeight, etc). However, the |
levithomason
changed the title
atMention prop
3 participants
atMention text can be of 2 types, myself or another person
Example
Usage