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

Migrate changeCSS() to CSS variable in utils/utils.jsx, ln. 700 #16028

Closed
deanwhillier opened this issue Oct 19, 2020 · 2 comments
Closed

Migrate changeCSS() to CSS variable in utils/utils.jsx, ln. 700 #16028

deanwhillier opened this issue Oct 19, 2020 · 2 comments

Comments

@deanwhillier
Copy link
Contributor

deanwhillier commented Oct 19, 2020

Context:

We are working on replacing our older custom theme implementation (applyTheme()) currently used to change component colors throughout the webapp to match the selected theme. We have decided to utilize CSS variables as a replacement and so we now need to replace all occurances of changeCSS() within the applyTheme() function with corresponding CSS variables directly in the CSS files. This ticket is part of a larger campaign.

Migration TL;DR

In the Mattermost Webapp repo, go to line number 700 in the file utils/utils.jsx (link). You should see the following (might not be the exact line number):

changeCss('.app*_body .post-create_*container .post-create-body .send-button.disabled i', 'color:' <ins> changeOpacity(theme.centerChannelColor, 0.4));
  • Work off the branch campaign/applytheme_center-channel-color
  • The indicated line number may not be accurate but should be close
  • Replace the Javascript variable theme.centerChannelColor with the CSS variable --center-channel-color from the line above
  • Create a PR against the branch campaign/applytheme_center-channel-color when ready (not 'Master')

Additional details on how to migrate

For this ticket, we will be replacing the use of the Javascript variable theme.centerChannelColor from the above line of code with the CSS variable --center-channel-color. Please work off the branch campaign/applytheme_center-channel-color to avoid conflicts with other tickets for the same campaign. The line number may not be the same as when this ticket was created, but should be close.

You can use these steps to help determine where to apply the new CSS variable followed by removing the call to changeCSS() from utils/utils.jsx:

  1. Make note of the CSS statement from the first argument to changeCSS and specifically the final selector (eg. .app__body .sidebar--menu)
  2. Using your browsers dev tools, locate the element targeted by this CSS statement in the webapp
  3. With the element selected in your dev tools, use the style browser to get an idea of how styles are currently applied to this element. This should help you track down where in the source code you will need to add the appropriate CSS variable in order to replace the style identified in the second argument to changeCSS() above (eg. 'background:' </ins> theme.sidebarBg)
  4. Locate the appropriate file(s) within the source code where edits will need to be made (eg. sass/layout/_sidebar-menu.scss)
  5. Make the necessary changes/additions to the CSS to apply the CSS variable and then remove the original JS line (shown above) from utils/utils.jsx.
  6. Create a pull request with your changes against the branch campaign/applytheme_center-channel-color. We will be using this branch to review and QA changes in batches instead of on each individual ticket in the campaign given the number of overall tickets for this campaign.

Notes:

  • Some instances of changeCSS() affect the styles of sections of the UI that might not be visible by default. Remember to check modals, pop-ups, menus and different interaction states (hover, active, etc.) as necessary based on the particular line you are working on
  • Some instances of changeCSS() include the use of a custom changeOpacity() function when applying colours that need to be partially transparent. These instances will need to be replaced with the rgba CSS function. For example, changeOpacity(theme.sidebarBg, 0.3)) in utils/utils.jsx would become rgba(var(--sidebar-bg-rgb), 0.3) in CSS. Appending -rgb to the CSS variable name is necessary when using one of the theme variables to work with the rgba CSS function.
  • Some UI elements in the desktop web view will actually be different elements in the mobile web view, eg. the desktop and mobile primary/hamburger menus have completely separate markup
  • Please be sure to test the primary responsive breakpoint widths after migration (Desktop: 1920px, Tablet: 960px, Mobile: 768px)
  • Please be sure to test changing themes after migration to ensure the new CSS variable is being applied correctly. Switching back and forth between light and dark themes would be a good way to help determine this. Responsive breakpoints should also be tested with different themes.
  • It is possible that an existing changeCSS() line is no longer valid, in which case, just removing the line as part of your PR is a perfectly valid solution.

Example of migrated code

PR #6655

  • This PR includes several commits that each address a slightly different CSS statement in order to provide several examples of differing situations and how they can be migrated. We suggest taking a look at each to get an idea of possible differences you could run across and how the resulting migration was accomplished.

Questions

Feel free to ask for help by messaging either @deanwhillier or @hmhealey or by posting in the ~webapp channel on the Mattermost Community server.


If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

JIRA ticket: https://mattermost.atlassian.net/browse/MM-29842

@oliverJurgen
Copy link

I would like to take this

@hanzei
Copy link
Contributor

hanzei commented Oct 29, 2020

@oliverJurgen It seems like @1jz already submitted mattermost/mattermost-webapp#6915 to fix this issue. I'm sorry we made you think that this ticket was up for grabs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants