[MM-54367] Apply scope to the menu class #213

kyeongsoosoo · 2023-09-15T14:16:51Z

Summary

Related conversation: https://community.mattermost.com/core/pl/cfk9y3o8jpy9tn4dhz4cd4ioqr

I found that the Todo plugin's css affects the entire Menu component.
I applied scope to the Menu component.

Ticket Link

Fixes mattermost/mattermost#24455
Jira https://mattermost.atlassian.net/browse/MM-54367

kyeongsoosoo · 2023-09-15T14:23:23Z

I found that the Button and Chip component have the same bugs. (https://github.com/mattermost/mattermost-plugin-todo/blob/a309a7ab63f0ea0ec7b91e7f46b22045e84ee08e/webapp/src/widget/buttons/button.scss#L1)
Would it be better to fix them together in this PR?

larkox · 2023-09-15T14:36:46Z

webapp/src/widget/menu.scss

-.Menu,
-.SubMenuOption .SubMenu {
+.TodoMenu,
+.SubTodoMenuOption .SubTodoMenu {


I see changes in the code for Menu->TodoMenu. Is there any change for SubMenuOption->SubTodoMenuOption needed?

I think it's not needed now. I added it for the future use-case.

If there is no class name being used in the components SubTodoMenuOption or SubTodoMenu, then I think they should be removed from the scss file to avoid confusion. Likely, yagni

@mickmister I removed the unused classnames.

mickmister

Thanks for catching this and implementing the fix @kyeongsoosoo 👍

The changes LGTM. I just have one non-blocking suggestion to remove the class names from the scss file that are not being used in the React code SubTodoMenuOption and SubTodoMenu

mickmister · 2023-09-19T20:55:30Z

webapp/src/widget/menuWrapper.tsx

@@ -19,7 +19,7 @@ const MenuWrapper = React.memo((props: Props) => {
    const [open, setOpen] = useState(Boolean(props.isOpen));

    if (!Array.isArray(props.children) || props.children.length !== 2) {
-        throw new Error('MenuWrapper needs exactly 2 children');
+        throw new Error('TodoMenuWrapper needs exactly 2 children');


The component should probably be renamed to TodoMenuWrapper if we change this error message

I reverted it back to MenuWrapper to keep it the same as the other components.

mickmister · 2023-09-19T21:01:20Z

webapp/src/widget/menu.scss

-.Menu,
-.SubMenuOption .SubMenu {
+.TodoMenu,
+.SubTodoMenuOption .SubTodoMenu {


If there is no class name being used in the components SubTodoMenuOption or SubTodoMenu, then I think they should be removed from the scss file to avoid confusion. Likely, yagni

mickmister · 2023-09-19T21:03:22Z

webapp/src/widget/menu.scss

 .status-dropdown-menu {
-    > .Menu {
+    > .TodoMenu {


Unrelated to this PR, but it seems wrong to assume that the plugin's TodoMenu component will always be a next-level descendant of the element that has status-dropdown-menu class name

I thought it would be okay because position is applied separately to the TodoMenu classname.
https://github.com/mattermost/mattermost-plugin-todo/blob/a309a7ab63f0ea0ec7b91e7f46b22045e84ee08e/webapp/src/widget/menu.scss#L7-L10

Right, but .status-dropdown-menu is managed by the core webapp. At any point, the webapp could restructure itsrlf and do one of:

Remove that class name altogether

Change its component structure to have a component in between .status-dropdown-menu and the plugin's component

Both of these would make it so our plugin styles don't get used. The alternative is to not have the > in the selector

AayushChaudhary0001

@mickmister @kyeongsoosoo
The issue has been solved in this PR. The UI does not get hampered through this fix, but the user is still able to resize the LHS while the channel menu is opened.
Steps to reproduce:-

Open the channel menu.
Try to resize the LHS.

The user is able to resize the LHS which contrasts the expected result in the issue.

mickmister · 2023-09-27T14:24:26Z

@AayushChaudhary0001 I think the issue you point out may be unrelated to this PR. If the issue the PR intends to fix is fixed by this PR, can you please approve it? We can make a separate issue for the main repository if there's an unrelated issue

mickmister · 2023-09-27T14:24:41Z

@kyeongsoosoo Heads up that there are some linting issues to resolve on the PR

codecov-commenter · 2023-09-27T16:55:27Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (a309a7a) 6.42% compared to head (b647dc5) 6.42%.

❗ Current head b647dc5 differs from pull request most recent head 9f5404d. Consider uploading reports for the commit 9f5404d to get more accurate results

Additional details and impacted files

@@          Coverage Diff           @@
##           master    #213   +/-   ##
======================================
  Coverage    6.42%   6.42%           
======================================
  Files          11      11           
  Lines        1712    1712           
======================================
  Hits          110     110           
  Misses       1594    1594           
  Partials        8       8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kyeongsoosoo · 2023-09-27T17:11:44Z

/update-branch

kyeongsoosoo · 2023-09-27T17:25:20Z

@mickmister Could you help me re-run the test? I fixed the linting issue.
The issue was on the server side. It's not PR related, but it was a minor change, so I fixed it.

mickmister · 2023-09-27T17:32:22Z

@kyeongsoosoo Thanks for providing a fix 👍

AayushChaudhary0001 · 2023-09-28T08:19:51Z

@mickmister Yes, I am approving this PR and we can create a different issue for this, but in the description of the PR, the expected result in the mentioned ticket should be similar to what has been tested, so kindly update that.

mickmister · 2023-09-28T15:19:43Z

@kyeongsoosoo Merging this, as it's obvious the issue that this plugin was causing is fixed. Is it your understanding that mattermost/mattermost#24455 is solved, or is there additional work to be done in the main repo to completely resolve the issue?

kyeongsoosoo · 2023-10-14T16:15:25Z

@mickmister Sorry for the late reply.
I think the ticket is solved. The issue mentioned above was not in the spec at the time of implementation, so I don't think it is a bug that should be fixed in this ticket.

apply scope to the menu class

b647dc5

kyeongsoosoo requested a review from larkox as a code owner September 15, 2023 14:16

larkox requested a review from mickmister September 15, 2023 14:35

larkox approved these changes Sep 15, 2023

View reviewed changes

larkox mentioned this pull request Sep 15, 2023

Release 0.7.1 #212

Merged

mickmister approved these changes Sep 19, 2023

View reviewed changes

mickmister added the 3: QA Review Requires review by a QA tester label Sep 19, 2023

mickmister requested a review from AayushChaudhary0001 September 19, 2023 21:06

AayushChaudhary0001 reviewed Sep 20, 2023

View reviewed changes

marianunez requested a review from yasserfaraazkhan September 26, 2023 13:14

marianunez removed the request for review from yasserfaraazkhan September 27, 2023 14:27

hanzei added the Awaiting Submitter Action Blocked on the author label Sep 27, 2023

kyeongsoosoo added 2 commits September 28, 2023 01:50

change error message

5f590c7

remove unused classnames

7182d14

kyeongsoosoo added 2 commits September 28, 2023 02:05

fix lint

72251f3

update branch

9f5404d

mickmister approved these changes Sep 27, 2023

View reviewed changes

mickmister removed the Awaiting Submitter Action Blocked on the author label Sep 27, 2023

AayushChaudhary0001 approved these changes Sep 28, 2023

View reviewed changes

mickmister merged commit 1aa947c into mattermost-community:master Sep 28, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-54367] Apply scope to the menu class #213

[MM-54367] Apply scope to the menu class #213

kyeongsoosoo commented Sep 15, 2023

kyeongsoosoo commented Sep 15, 2023

larkox Sep 15, 2023

kyeongsoosoo Sep 15, 2023 •

edited

Loading

mickmister Sep 19, 2023

kyeongsoosoo Sep 27, 2023

mickmister left a comment

mickmister Sep 19, 2023

kyeongsoosoo Sep 27, 2023

mickmister Sep 19, 2023

mickmister Sep 19, 2023

kyeongsoosoo Sep 27, 2023

mickmister Sep 28, 2023

AayushChaudhary0001 left a comment •

edited by Kshitij-Katiyar

Loading

mickmister commented Sep 27, 2023

mickmister commented Sep 27, 2023

codecov-commenter commented Sep 27, 2023 •

edited

Loading

kyeongsoosoo commented Sep 27, 2023

kyeongsoosoo commented Sep 27, 2023

mickmister commented Sep 27, 2023 •

edited

Loading

AayushChaudhary0001 commented Sep 28, 2023

mickmister commented Sep 28, 2023 •

edited

Loading

kyeongsoosoo commented Oct 14, 2023

[MM-54367] Apply scope to the menu class #213

[MM-54367] Apply scope to the menu class #213

Conversation

kyeongsoosoo commented Sep 15, 2023

Summary

Ticket Link

kyeongsoosoo commented Sep 15, 2023

Choose a reason for hiding this comment

kyeongsoosoo Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AayushChaudhary0001 left a comment • edited by Kshitij-Katiyar Loading

Choose a reason for hiding this comment

mickmister commented Sep 27, 2023

mickmister commented Sep 27, 2023

codecov-commenter commented Sep 27, 2023 • edited Loading

Codecov Report

kyeongsoosoo commented Sep 27, 2023

kyeongsoosoo commented Sep 27, 2023

mickmister commented Sep 27, 2023 • edited Loading

AayushChaudhary0001 commented Sep 28, 2023

mickmister commented Sep 28, 2023 • edited Loading

kyeongsoosoo commented Oct 14, 2023

kyeongsoosoo Sep 15, 2023 •

edited

Loading

AayushChaudhary0001 left a comment •

edited by Kshitij-Katiyar

Loading

codecov-commenter commented Sep 27, 2023 •

edited

Loading

mickmister commented Sep 27, 2023 •

edited

Loading

mickmister commented Sep 28, 2023 •

edited

Loading