Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Conversation

@Johennes
Copy link
Contributor

@Johennes Johennes commented Apr 2, 2022

Requires matrix-org/matrix-js-sdk#2272
For element-hq/element-web#21671
MSC: matrix-org/matrix-spec-proposals#3765

Fixes element-hq/element-web#5180

This allows to enter MD / HTML for room topics in the following places:

  • /topic command
  • Room settings overlay
  • Space settings overlay

The HTML rendering is displayed in these places:

  • /topic command
  • Room header
  • Space home

Fixes: element-hq/element-web#5180

Screenshots (after)

Screenshot 2022-04-02 at 21 00 06
Screenshot 2022-04-02 at 20 59 19
Screenshot 2022-04-02 at 20 59 35
Screenshot 2022-04-02 at 20 59 49

Screenshots (before)

Screenshot 2022-04-02 at 21 03 16


Here's what your changelog entry will look like:

✨ Features

Preview: https://pr8215--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

@Johennes Johennes requested a review from a team as a code owner April 2, 2022 19:04
@Johennes Johennes marked this pull request as draft April 2, 2022 19:04
@codecov
Copy link

codecov bot commented Apr 3, 2022

Codecov Report

Merging #8215 (78f8d38) into develop (3e31fdb) will increase coverage by 0.02%.
The diff coverage is 55.81%.

❗ Current head 78f8d38 differs from pull request most recent head ec16b10. Consider uploading reports for the commit ec16b10 to get more accurate results

@@             Coverage Diff             @@
##           develop    #8215      +/-   ##
===========================================
+ Coverage    30.85%   30.87%   +0.02%     
===========================================
  Files          893      893              
  Lines        50793    50824      +31     
  Branches     12928    12941      +13     
===========================================
+ Hits         15671    15691      +20     
- Misses       35122    35133      +11     
Impacted Files Coverage Δ
src/SlashCommands.tsx 4.26% <0.00%> (-0.03%) ⬇️
src/components/structures/SpaceRoomView.tsx 4.36% <ø> (ø)
...onents/views/room_settings/RoomProfileSettings.tsx 0.00% <0.00%> (ø)
...omponents/views/spaces/SpaceSettingsGeneralTab.tsx 2.17% <0.00%> (-0.05%) ⬇️
src/settings/Settings.tsx 57.14% <ø> (ø)
src/HtmlUtils.tsx 41.33% <71.42%> (+2.71%) ⬆️
src/components/views/elements/RoomTopic.tsx 78.57% <83.33%> (+5.84%) ⬆️
src/components/views/rooms/RoomHeader.tsx 72.94% <100.00%> (ø)
src/editor/serialize.ts 63.91% <100.00%> (+0.75%) ⬆️

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.

generally seems fine - just a few points where we might want to consider a different approach.

@Johennes Johennes requested a review from turt2live April 15, 2022 09:29
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.

overall looks good - just the minor nitpicks here + a comment in the CSS please :)

@Johennes Johennes requested a review from turt2live April 20, 2022 19:36
@Johennes
Copy link
Contributor Author

All comments addressed. There is an open discussion on the MSC about whether or not m.topic and m.message should be allowed to deviate in future (and if not, that would affect the key in the event content). I'm not sure if that should block this PR though.

@turt2live
Copy link
Member

something appears to have gone very wrong with your merge - suggest a reset & rebase before review can really be completed.

@turt2live turt2live removed their request for review May 2, 2022 00:56
Setting MD / HTML supported:
- /topic command
- Room settings overlay
- Space settings overlay

Display of MD / HTML supported:
- /topic command
- Room header
- Space home

Based on extensible events as defined in [MSC1767]

Fixes: element-hq/element-web#5180
Signed-off-by: Johannes Marbach <[email protected]>

[MSC1767]: matrix-org/matrix-spec-proposals#1767
@Johennes Johennes force-pushed the feature/html-topic branch from b5a0b24 to 78f8d38 Compare May 2, 2022 18:13
@Johennes
Copy link
Contributor Author

Johennes commented May 2, 2022

something appears to have gone very wrong with your merge - suggest a reset & rebase before review can really be completed.

Yeah, I screwed up after pulling in the suggested and applied changes from GitHub. The final diff was actually correct, just the history messed up. Unfortunately, there were a myriad of commits inbetween so the only sensible way I found to fix this was to reset and commit the diff as a single commit. Hope that's ok.

@github-actions github-actions bot temporarily deployed to Netlify May 2, 2022 18:18 Destroyed
@Johennes Johennes requested a review from turt2live May 2, 2022 18:27
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.

generally still seems fine - just nervous about landing such a change, still.

For the CSS: if we're not copy/pasting the styles from somewhere, please put a comment surrounding the whole block to note that the !important overrides are actually doing a thing.

@turt2live
Copy link
Member

also you have CI failures + merge conflicts, sorry. Please do a merge commit if possible to avoid having to have the whole thing re-reviewed :)

@Johennes
Copy link
Contributor Author

Johennes commented May 9, 2022

also you have CI failures + merge conflicts, sorry. Please do a merge commit if possible to avoid having to have the whole thing re-reviewed :)

Builds are green except for the e2e tests. I'm not sure if those failures are due to my changes though as the log doesn't seem related at all:

2022-05-09T18:27:50.219Z  * enterprise_erin logs out:
2022-05-09T18:27:50.219Z    * enterprise_erin navigates to user menu ... done
2022-05-09T18:27:50.269Z    * enterprise_erin clicks the 'Sign Out' button ... done
2022-05-09T18:27:50.323Z  * enterprise_erin waits for redirect to config.json (as external page) ... done
failure:  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ '1'
- 'some-new-version'
    at /__w/matrix-react-sdk/matrix-react-sdk/test/end-to-end-tests/lib/src/scenarios/update.js:58:25
    at Generator.next (<anonymous>)
    at fulfilled (/__w/matrix-react-sdk/matrix-react-sdk/test/end-to-end-tests/lib/src/scenarios/update.js:20:58)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:95:5) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: '1',
  expected: 'some-new-version',
  operator: 'strictEqual'
}

I also don't have rights to re-run builds so I cannot retrigger them.

@Johennes Johennes requested a review from turt2live May 9, 2022 18:37
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.

almost there - apologies for the delay in the review.

please note a reopened conversation on the previous review as well.

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.

:shipit:

@Johennes
Copy link
Contributor Author

Thanks @turt2live! 🙌

And PS: I don't have write access so could you please merge this for me?

@t3chguy
Copy link
Member

t3chguy commented May 30, 2022

@Johennes lets hold this off until tomorrow after the RC, we avoid merging large changes right before an RC cut

@t3chguy t3chguy added the X-Blocked The PR cannot move forward in any capacity until an action is made label May 30, 2022
@Johennes
Copy link
Contributor Author

Johennes commented Jun 7, 2022

Is this safe to merge now?

@turt2live turt2live removed the X-Blocked The PR cannot move forward in any capacity until an action is made label Jun 7, 2022
@turt2live
Copy link
Member

I'm going to land this without working cypress tests as I believe the CI is just horribly confused about branches or context or something. Will monitor develop.

@turt2live turt2live disabled auto-merge June 7, 2022 20:20
@turt2live turt2live merged commit abd39c6 into matrix-org:develop Jun 7, 2022
@nmscode nmscode mentioned this pull request Jul 26, 2023
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handle markdown in topics

3 participants