in some places
ClosedPublic
Actions

Authored by michal on Oct 10 2022, 3:58 AM.

Details

Reviewers

tomek
atul
• abosh

Commits

rCOMM824b816c8a1b: [web] Use <a> instead of <div> in some places

Summary

Part of ENG-843
According to the linked article that @tomek linked in the comments of the task, we should use <a> in some places (where it brings user to a new chat/ screen).

Because of these changes we no longer use the Button for the settings button, so the round variant is no longer used and could be removed.

Test Plan

Check if all changed buttons look the same. Check if the changed css classes are used anywhere else. Check if rest of the app looks the same.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

michal created this revision.Oct 10 2022, 3:58 AM

Herald added a subscriber: ashoat. · View Herald TranscriptOct 10 2022, 3:59 AM

michal added a parent revision: D5324: [web] Use Button for relationship buttons.Oct 10 2022, 4:01 AM

Harbormaster completed remote builds in B12690: Diff 17440.Oct 10 2022, 4:08 AM

michal requested review of this revision.Oct 10 2022, 4:08 AM

Looks good at a high level, adding @tomek as blocking to take another look

tomek accepted this revision.Oct 11 2022, 4:13 AM

tomek added inline comments.

web/chat/chat-thread-list.css
1 ↗	(On Diff #17440)	I don't think we need to specify node type
web/components/button.css
76–82 ↗	(On Diff #17440)	I can see that this is removed because `.settingsIcon` class was introduced and we no longer use `Button` in `community-picker`. It might be good to explain that in the summary. Also, this diff does three things: Replaces `div`s with `a`s in a cople of places Replaces `Button` with `a` in one place Removes unused `Button` variant I think it would be better to have two separate diffs, one with 1. and one with 2. and 3. (but keeping this all together also made some sense because the theme of this diff is to replace things with `a`). Let's keep it as it is.