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.

Sign up for GitHub

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

Jump to bottom

MSC2422: Allow color on font tag #2422

Merged
merged 6 commits into from
Mar 24, 2020
Merged

MSC2422: Allow color on font tag #2422

merged 6 commits into from
Mar 24, 2020

Conversation

deepbluev7
Copy link
Contributor

Signed-off-by: Nicolas Werner nicolas.werner@hotmail.de

Rendered

Just a minor thing I noticed, when RiotX added a /rainbow command and I wanted to add that to nheko.

@deepbluev7
MSC2422: Allow color on font tag
b47de28 
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
@turt2live turt2live added proposal A matrix spec change proposal proposal-in-review labels Jan 24, 2020
@Sorunome
Copy link
Contributor

Font tag already has color? At least riot parses it and /rainbow outputs with color tag.....is this riot going off-spec?

@deepbluev7
Copy link
Contributor Author

It is Riot, RiotX and Spectral at least, that are not following the spec here. Which is why I am proposing this. All clients already seem to allow and use color, so why not make it valid?

@turt2live
Copy link
Member

(It can't go off-spec if the spec is just a suggestion)

@deepbluev7
Copy link
Contributor Author

(fair enough. I would still prefer that to be part of the spec, as clients are otherwise encouraged to sanitize attributes, that are not whitelisted and this would lead to incompatibilities.)

@turt2live turt2live self-requested a review March 4, 2020 23:01
turt2live
turt2live reviewed Mar 5, 2020
View reviewed changes
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Otherwise seems fine, thanks!

proposals/2422-allow-color-attribute-on-font-tag.md Outdated Show resolved Hide resolved
proposals/2422-allow-color-attribute-on-font-tag.md Outdated Show resolved Hide resolved
@turt2live
Copy link
Member

Assuming the 2 relatively minor comments are addressed, I think this is fine to go through without much further comment. Stuff like this is actually changing a recommendation of the spec so it could in theory skip the MSC process, but we're here now so we might as well go with the process instead (it also serves as a good discussion point, if this was more controversial of a change).

So, with that being said:

@mscbot fcp merge

@turt2live

This comment has been minimized.

Sign in to view
@turt2live turt2live added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. and removed proposal-in-review labels Mar 5, 2020
deepbluev7 and others added 2 commits March 5, 2020 21:02
@deepbluev7 @turt2live
Update proposals/2422-allow-color-attribute-on-font-tag.md
438ff8f 
Co-Authored-By: Travis Ralston <travpc@gmail.com>
@deepbluev7 @turt2live
Update proposals/2422-allow-color-attribute-on-font-tag.md
6b2752c 
Co-Authored-By: Travis Ralston <travpc@gmail.com>
@mscbot
Copy link
Collaborator

mscbot commented Mar 5, 2020 •
edited by dbkr
Loading

Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people:

Concerns:

None listed.

Once at least 75% of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

KitsuneRal
KitsuneRal reviewed Mar 6, 2020
View reviewed changes
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

Quite good overall; one nice-to-have below.

proposals/2422-allow-color-attribute-on-font-tag.md Outdated Show resolved Hide resolved
@richvdh richvdh self-requested a review March 6, 2020 11:03
@richvdh richvdh requested review from richvdh and removed request for richvdh March 7, 2020 17:48
@deepbluev7 @richvdh
Update proposals/2422-allow-color-attribute-on-font-tag.md
2d85422 
Co-Authored-By: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@mscbot
Copy link
Collaborator

mscbot commented Mar 19, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Mar 19, 2020
@deepbluev7 @auscompgeek
Update proposals/2422-allow-color-attribute-on-font-tag.md
28dc6a0 
Apply typo fix as suggested.

Co-Authored-By: David Vo <auscompgeek@users.noreply.github.com>
@mscbot
Copy link
Collaborator

mscbot commented Mar 24, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

@mscbot mscbot added finished-final-comment-period and removed final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Mar 24, 2020
@turt2live turt2live merged commit b8b0e0d into matrix-org:master Mar 24, 2020
@turt2live turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels Mar 24, 2020
@turt2live turt2live added the kind:maintenance MSC which clarifies/updates existing spec label Apr 20, 2020
@turt2live turt2live self-assigned this May 15, 2020
turt2live added a commit that referenced this pull request May 15, 2020
@turt2live
Spec spoilers and color attribute allowance
27db337 
Spoilers MSC: #2010
`color` attribute MSC: #2422
@turt2live turt2live mentioned this pull request May 15, 2020
Closed
@turt2live
Copy link
Member

turt2live commented May 15, 2020 •
edited
Loading

Spec PR: #2549

@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels May 15, 2020
@turt2live turt2live mentioned this pull request May 15, 2020
Closed
turt2live added a commit that referenced this pull request Apr 6, 2021
@turt2live
Incorporate spoilers and color tag allowance
38ea7fd 
Specs [MSC2010](#2010)
Specs [MSC2557](#2557)
Specs [MSC2422](#2422)
Obsoletes #2549
Built upon #3094
@turt2live turt2live mentioned this pull request Apr 6, 2021
Merged
@turt2live turt2live added merged A proposal whose PR has merged into the spec! and removed spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Apr 6, 2021
@turt2live
Copy link
Member

Merged 🎉

richvdh pushed a commit that referenced this pull request Aug 23, 2021
@turt2live @richvdh
Incorporate spoilers and color tag allowance
bb5d351 
Specs [MSC2010](#2010)
Specs [MSC2557](#2557)
Specs [MSC2422](#2422)
Obsoletes #2549
Built upon #3094
richvdh pushed a commit that referenced this pull request Aug 27, 2021
@turt2live @richvdh
Incorporate spoilers and color tag allowance
a0345ea 
Specs [MSC2010](#2010)
Specs [MSC2557](#2557)
Specs [MSC2422](#2422)
Obsoletes #2549
Built upon #3094
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@auscompgeek auscompgeek auscompgeek left review comments

@turt2live turt2live turt2live left review comments

@richvdh richvdh richvdh left review comments

@KitsuneRal KitsuneRal KitsuneRal approved these changes

Assignees

@turt2live turt2live

Labels
disposition-merge kind:maintenance MSC which clarifies/updates existing spec merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@deepbluev7 @Sorunome @turt2live @mscbot @auscompgeek @richvdh @KitsuneRal