Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

Campaign/applytheme sidebar header text color #7379

Merged
merged 6 commits into from
Jan 27, 2021
Merged

Campaign/applytheme sidebar header text color #7379

merged 6 commits into from
Jan 27, 2021

Conversation

deanwhillier
Copy link
Contributor

Summary

ApplyTheme Campaign: This PR completes the merge of the theme.sidebarHeaderTextColor commits from the campaign branch.

Dev approval for the changes was completed on the original PR(s), so we are just looking for rubber stamp approvals by dev's to satisfy merge requirements.

As QA was skipped on PR merges into campaign branches, this PR will need proper QA before merging. See below for details on what to QA for this PR.

Ticket Link

Related Pull Requests

See tickets for related pull requests

xaylax and others added 4 commits January 22, 2021 11:57
@xaylax @deanwhillier
[GH-15875] Migrate changeCSS() to CSS variable in utils/utils.jsx, ln…
c7be869 
…. 564 (#6745)
@haardikdharma10 @deanwhillier
[MM-29461] Migrate changeCSS() to CSS variable in utils/utils.jsx in …
0c49464 
…560. (#6720)

* Migrate changeCSS() to CSS variable in utils/utils.jsx in 560.

* Update _navigation.scss
@ashutosh97 @deanwhillier
[GH-15873] Migrate changeCSS() to CSS variable in utils/utils.jsx, ln…
408a980 
…. 562 (#6703)

* Switch from cssChange() for team-btn border-color

* Moved css to existing class
@arc9693 @deanwhillier
[GH-15871] Migrate changeCSS() to CSS variable in utils/utils.jsx, ln…
b674b3d 
…. 559 (#6701)
@deanwhillier deanwhillier added 2: Dev Review Requires review by a core commiter Work in Progress Not yet ready for review 3: QA Review Requires review by a QA tester labels Jan 22, 2021
@deanwhillier deanwhillier mentioned this pull request Jan 22, 2021
Closed
deanwhillier
deanwhillier commented Jan 22, 2021
View reviewed changes
sass/layout/_navigation.scss
@@ -90,6 +90,7 @@
&.navbar-right__icon {
@include border-radius(50px);
display: flex;
background: rgba(var(--sidebar-header-text-color-rgb), 0.2);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jgilliam17, this applies to the background colour for the top right navbar buttons in the mobile view.

sass/layout/_navigation.scss
@@ -104,7 +105,7 @@
}

.icon-bar {
background: $white;
background: var(--sidebar-header-text-color);
Copy link
Contributor Author

@deanwhillier deanwhillier Jan 22, 2021 •
edited by devinbinnie
Loading

Choose a reason for hiding this comment

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

@jgilliam17, as far as I can tell, this selector isn't being used anymore.

sass/layout/_navigation.scss
Comment on lines +141 to +143
.app__body & {
color: var(--sidebar-header-text-color)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jgilliam17, this affects the text colour of the header bar in mobile view

sass/responsive/_mobile.scss
@@ -1370,7 +1370,7 @@
.search-bar__container {
@include flex(0 0 50px);
background: $primary-color;
color: $white;
color: var(--sidebar-header-text-color);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jgilliam17, this is supposed to set the text colour for the search bar header in mobile, but unfortunately the input field doesn't inherit it and so is not being styled properly. We have a separate ticket to handle that.

@deanwhillier deanwhillier removed the Work in Progress Not yet ready for review label Jan 22, 2021
@devinbinnie devinbinnie removed the 2: Dev Review Requires review by a core commiter label Jan 25, 2021
@jgilliam17 jgilliam17 added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Jan 27, 2021
jgilliam17
jgilliam17 approved these changes Jan 27, 2021
View reviewed changes
Copy link
Contributor

@jgilliam17 jgilliam17 left a comment

Choose a reason for hiding this comment

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

Thanks for the test steps @deanwhillier
Tested, looks good to merge.

  • Verified background color of navbar buttons, header bar in mobile view - as expected. Text color is search bar in mobile view is not fixed here, will be handled in a different ticket.

@jgilliam17 jgilliam17 added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Jan 27, 2021
@mm-cloud-bot
Copy link

Test server destroyed

@deanwhillier
Copy link
Contributor Author

/update-branch

@deanwhillier deanwhillier merged commit eec30f1 into master Jan 27, 2021
@deanwhillier deanwhillier deleted the campaign/applytheme_sidebar-header-text-color branch January 27, 2021 17:40
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jan 27, 2021
@amyblais amyblais added this to the v5.33 milestone Jan 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nevyangelova nevyangelova nevyangelova approved these changes

@devinbinnie devinbinnie devinbinnie approved these changes

@jgilliam17 jgilliam17 jgilliam17 approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation
Projects
None yet
Milestone
v5.33.0
Development

Successfully merging this pull request may close these issues.