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.

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

Merge HeaderButtons.tsx and RoomHeaderButtons.tsx #25036

Closed
luixxiul opened this issue Apr 5, 2023 · 1 comment
Closed

Merge HeaderButtons.tsx and RoomHeaderButtons.tsx #25036

luixxiul opened this issue Apr 5, 2023 · 1 comment
Labels
A-Room-View O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Tolerable Low/no impact on users T-Enhancement

Comments

@luixxiul
Copy link

luixxiul commented Apr 5, 2023

Your use case

What would you like to do?

I would like to see HeaderButtons.tsx and RoomHeaderButtons.tsx merged, if they essentially do the same thing.

Why would you like to do it?

There are several components which seem to be related to display buttons on the room header: RoomHeader, RoomHeaderButtons, HeaderButtons, and HeaderButton.

Currently HeaderButton and HeaderButtons are used on RoomHeaderButtons.tsx only, and since RoomHeaderButtons is used on RoomHeader.tsx only, it means that there is a simple hierarchy as below:

RoomHeader
  - …
  - RoomHeaderButtons
    - HeaderButtons
      - HeaderButton 

HeaderButtons has been used to render buttons which display content inside BaseCard if they are clicked, for example, pinned message card, a thread, and a room information card, which I think is because those buttons are were tagged with ARIA tab role instead of button (the tab role had been in fact not effective, because there was not an associated element tagged with tabpanel. Please see here for more information: #25016).

Because HeaderButtons are used only on the room header, it should be fine to merge HeaderButtons.tsx with RoomHeaderButtons.tsx to simplify the structure such as below:

RoomHeader
  - …
  - RoomHeaderButtons
      - RoomHeaderButton 

Now the the ARIA "tablist" role has been removed from HeaderButtons by matrix-org/matrix-react-sdk#10628, there is no point of keeping HeaderButtons besides RoomHeaderButtons, unless there would be something like "SpaceHeader".

How would you like to achieve it?

Have you considered any alternatives?

No response

Additional context

No response

@andybalaam andybalaam added S-Tolerable Low/no impact on users O-Uncommon Most users are unlikely to come across this or unexpected workflow A-Room-View labels Apr 5, 2023
@luixxiul
Copy link
Author

There are also some components on RoomHeader.tsx for displaying buttons on the room header, so the code to display them scatters over those four files.

@luixxiul luixxiul closed this as not planned Won't fix, can't repro, duplicate, stale Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Room-View O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Tolerable Low/no impact on users T-Enhancement
Projects
None yet
Development

No branches or pull requests

2 participants