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.

Sign up for GitHub

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

Jump to bottom

Focalboard UI Fixes #1194

Merged
merged 8 commits into from
Sep 14, 2021
Merged

Focalboard UI Fixes #1194

merged 8 commits into from
Sep 14, 2021

Conversation

asaadmahmood
Copy link
Contributor

@asaadmahmood asaadmahmood commented Sep 13, 2021 •
edited
Loading

Summary

Focalboard UI Fixes

Ticket Link

Fixes #1189
Fixes #1182
Fixes #1173
Fixes #1172
Fixes #1191
Fixes #1197

@asaadmahmood asaadmahmood requested a review from a team as a code owner September 13, 2021 18:09
@asaadmahmood asaadmahmood added 2: Dev Review Requires review by a core committer 2: QA Review Requires review by a QA tester labels Sep 13, 2021
@chenilim chenilim mentioned this pull request Sep 13, 2021
Closed
chenilim
chenilim approved these changes Sep 13, 2021
View reviewed changes
Copy link
Contributor

@chenilim chenilim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me.

mattermost-plugin/webapp/src/plugin.scss
@@ -9,9 +9,6 @@
z-index: 1000;

.Menu {
position: unset;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbishel, was there a reason this was needed in the original checkin? I'm worried removing it might break something else?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The min-width: unset; is not needed because the min-width: 180px; in menu.scss is being removed as well, see below.

However, the position: unset; removal does cause a slight difference in menu positioning between Channels and Boards.

Screen Shot 2021-09-13 at 5 23 11 PM

Screen Shot 2021-09-13 at 5 23 01 PM

sbishel
sbishel requested changes Sep 13, 2021
View reviewed changes
Copy link
Collaborator

@sbishel sbishel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the change in positioning, the other changes look good and everything works properly.

@asaadmahmood
Copy link
Contributor Author

@sbishel We should not override the position or the min/max width coming from the global header, otherwise a lot of things can get messed up.

That position is also getting messed up because we are overriding the global menus position based position: absolute we have on our .Menu.

I have however overriden it in my latest commit for now.
But we should remove the .focalboard-body appended to the scss selectors later.

Also the issue you're talking about should be fixed now.

Screenshot 2021-09-14 at 11 15 03 AM

@ogi-m
Copy link
Contributor

ogi-m commented Sep 14, 2021

Verified the fixes for:

But I'm still seeing the white space in the user modal
whitespace

Also opened an issue for the styling on the settings and custom status modals.

@asaadmahmood
Copy link
Contributor Author

@ogi-m Should be fixed now.

ogi-m
ogi-m approved these changes Sep 14, 2021
View reviewed changes
Copy link
Contributor

@ogi-m ogi-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @asaadmahmood , LGTM

@ogi-m ogi-m removed the 2: QA Review Requires review by a QA tester label Sep 14, 2021
@asaadmahmood asaadmahmood merged commit 1836f95 into mattermost-community:main Sep 14, 2021
@asaadmahmood asaadmahmood deleted the ui-issues branch September 14, 2021 15:58
asaadmahmood added a commit that referenced this pull request Sep 14, 2021
@asaadmahmood
Focalboard UI Fixes (#1194)
82e0ed0 
* 1173 - Show description fix

* 1191 - Octo-block Hover fix

* 1189 - Updating width of kanban header

* 1172 - Updating menu in the header

* Updating css

* Updating menu css

* Updating menu css
# Conflicts:
#	webapp/src/styles/main.scss
#	webapp/src/widgets/menu/menu.scss
@chenilim chenilim added the CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone label Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chenilim chenilim chenilim approved these changes

@ogi-m ogi-m ogi-m approved these changes

@sbishel sbishel sbishel approved these changes

Assignees
No one assigned
Labels
2: Dev Review Requires review by a core committer CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone
Projects
None yet
Milestone
No milestone
4 participants
@asaadmahmood @ogi-m @sbishel @chenilim