Add feature flag 'feature_new_room_decoration_ui' and segrate legacy UI component #11345
Conversation
germain-gg
added
the
T-Task
There was a problem hiding this comment.
Github is making the RoomHeader/LegacyRoomHeader thing impossible to review. Any chance you could try to convince it that RoomHeader is a new file and that LegacyRoomHeader is predominantly a move? Maybe we could forgo most of this change by having NewRoomHeader until such time when we replace the "Legacy" one and thus everything is straight renames/moves.
| "mx_RoomHeader_button": true, | ||
| "mx_RoomHeader_button--highlight": isHighlighted, | ||
| "mx_RoomHeader_button--unread": isUnread, | ||
| "mx_LegacyRoomHeader_button": true, |
There was a problem hiding this comment.
This implies this should be LegacyHeaderButton
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actually, these classes should not be existing in this file in the first place. I will be keeping this file with the current name and will not spend time refactoring code we're looking to remove.
| viewingCall={false} | ||
| activeCall={null} | ||
| /> | ||
| {SettingsStore.getValue("feature_new_room_decoration_ui") ? ( |
There was a problem hiding this comment.
Could we monitor/watch the setting instead of requiring a refresh?
There was a problem hiding this comment.
I'd rather skip this change altogether, i do not want to add more logic that i need to write tests for.
This is just pushing the problem further down the line. I'd rather keep it this way for now. |
Do you have an alternative to convince git/hub to simplify the diff? |
|
Unfortunately no. I have not changed any of the logic. |
|
The diff disagrees. Could you rebase into sensible commits? Maybe something like
|
germain-gg
force-pushed
the
germain-gg/25887
branch
from
9b1cbf0
to
3759042
Compare
|
@t3chguy Sure, done. Hopefully those two commits help you to have a better review. |
|
@germain-gg I don't think that quite worked - b99467b doesn't rename as it claims - it copies. Making the diff have an extra ~819 lines to review which is making it more difficult 3759042 also doesn't create the new room header as it claims, instead it just modifies an existing file. |
|
Well, i'm not sure how to make git change that. Further, but happy to hear suggestions |
|
In b99467b you created a new file instead of renaming, not sure how else to say how to fix it. A rename != copy, its a move |
|
@germain-gg like cfef892 + 4ff5ae9 but also for the pcss file |
germain-gg
force-pushed
the
germain-gg/25887
branch
from
3759042
to
355cef1
Compare
|
@germain-gg thanks so much, those 2 commits look great. Just add one which fixes whatever is upsetting CI - don't bother rebasing into the existing 2 commits - and once that last commit looks good I'll approve |
germain-gg
force-pushed
the
germain-gg/25887
branch
from
355cef1
to
3b47c68
Compare
|
done. the commits contain the diff for the file rename. |
|
@germain-gg while we wait for CI can you add it to labs.md? |
There was a problem hiding this comment.
LGTM once documented as per https://github.com/vector-im/element-web/blob/develop/docs/config.md#labs-flags
Thanks for wrangling the commits into sane shape
Fixes element-hq/element-web#25887
Adds a new labs flag
feature_new_room_decoration_uithat will be useful during the development of element-hq/element-web#25883Current UI changes:
Checklist
This change is marked as an internal change (Task), so will not be included in the changelog.