[MM-37838] - remove global header feature flag

[michelengelen]

Summary

remove all places where the global header feature flag is checked for.

Ticket Link

MM-37838

Related Pull Requests

mattermost/mattermost#18304

Release Note

NONE

[zefhemel]

I had kind of expected a larger scale clean-up. I thought we duplicated a lot of menu items to handle both a case where the global header would be disabled (options like "Account settings" in left-hand channel), but I suppose that remains for mobile?

[michelengelen]

@zefhemel we definitely need to clean up more of the intermittent stuff, but the ticket just says to remove the global header feature flag, soooo ...

IMO we should have 1-2 sprints for clean-up tasks, since there is way more to it than originally thought: custom selectors, getters, adjustments to existing styling and components, etc.

On top of that we have the implementation of compass components, which kind of brought its own struggles.

My impression is, that we will have an easier time once the compass-components get built in more and more, but for now I just removed the feature flag and the getter ✨

[zefhemel]

From the ticket

Remove the GlobalHeader feature flag altogether as well as any duplicate code that was there to support this flag in both states.

But if it's too much, we can do cleanup separately.

[michelengelen]

From the ticket

Remove the GlobalHeader feature flag altogether as well as any duplicate code that was there to support this flag in both states.

But if it's too much, we can do cleanup separately.

IMO we should do more small PRs, since those monolithic changes are pretty hard to review and have some other downcomes as well, but I am always open for discussion about this topic!

[nevyangelova]

@zefhemel @michelengelen I am sorry if I might have missed any discussions for removing the global header during my PTO but I wanted to talk about the whole clean-up a little more, since there is the mobile view issue of having to keep some of the menu items as they were on mobile, unless we do have a mobile view that incorporates global header. The new menu that we are planning to deliver would also play a large part in determining that approach. Otherwise let me know about the scope for this ticket that is agreed upon the team and I can review 👍 but I am leaning more towards splitting in smaller PR's as well.

[zefhemel]

Ok, I'll add this as a topic for tomorrow's planning meeting. For now let's just proceed with removing the flag in its most basic form.

[nevyangelova]

LGTM! Not sure if it should be a part of this PR or the next but maybe we can remove some of the checks globalHeaderEnabled && Utils.isMobile() or !this.props.mobile since they are checking for the same thing

[michelengelen]

nice catch @nevyangelova ... I will search all of those occurances and replace them. 🙇🏼‍♂️

[crspeller]

I think this PR needs to be updated given #8671

[michelengelen]

@crspeller I removed the usages of the getter where possible. We do need it in some places for computation purposes and conditional renderings, so unfortunately we need it for now

[stylianosrigas]

FYI spinwick is failing to create after the last commit. I am killing it and feel free to apply the label again to recreate it.

[mm-cloud-bot]

Test server destroyed

[michelengelen]

@deanwhillier, @crspeller and @jgilliam ... can we move on with this? kind reminder to re-review!

[crspeller]

I asked @deanwhillier to review given I think this reverts part of his pr #8671

Blocking on him.

[deanwhillier]

@michelengelen, @nevyangelova, @crspeller ... @crspeller is correct, we can't just add those two isMobile calls back into the mix as things will start breaking again between the desktop mode, mobile web mode not to mention the middle tablet ground. I took a bit of a closer look at the various implementations of isMobile and window resizing over the weekend and am currently working on a redux-integrated solution to consolidate them into a (hopefully) single, globally available solution that we can use to help in properly removing the Global Header feature flag. Stay tuned!

[deanwhillier]

Okay, so while I've been thinking and working through reactive solutions in JS to the isMobile() challenge for Global Header, I think we should just stick with the CSS solution for these reasons:

1. CSS Media Queries are meant for adjusting UI in these kinds of situations
2. CSS uses a separate thread from JS and doesn't get blocked by, or contribute to blocking of JS
3. Having simple markup hidden using display: none has almost no impact on the performance in-browser as the browser doesn't even consider the markup when rending
4. It is arguably less performant for React to remove/add the markup to the DOM than just getting CSS to show/hide it.
5. Mobile web-view will soon be updated to support the Global Header, so a complex alternative JS approach is IMO a waste of time
6. Most of Global header's functionality and markup is in components that are only rendered when triggered using buttons on the bar so won't even come into play.
7. If we decide that animations are needed at some point to transition the bar in and out, it is much easier to handle if the markup is still present (yes, I know there are still ways to do this with JS). This one is minor as it's hypothetical at this point, but ...

That said, I'm not abandoning the possibility of a reactive `isMobile()' Redux selector, I just don't think it's needed at this point to determine if the Global Header gets rendered or not. The minor benefits of not having the markup present outweigh the challenges of trying to responsibly accomplish removing the bar with JS instead of with CSS.

I would add the media query to the Global Header styled-component like so:

    position: relative;
    display: flex;
    flex-shrink: 0;
    align-items: center;
    justify-content: space-between;
    height: 40px;
    background: var(--global-header-background);
    border-bottom: solid 1px rgba(var(--center-channel-color-rgb), 0.08);
    color: rgba(var(--global-header-text-rgb), 0.64);
    padding: 0 12px;

    > * + * {
        margin-left: 12px;
    }

    @media screen and (max-width: 768px) {
        display: none;
    }

cc/ @crspeller, @michelengelen, @hmhealey, @nevyangelova

[hmhealey]

There's a lot of conflicts and commits on this branch, and it looks like it had master merged into it while it's pointed at the release branch. Did something get messed up between the two or is it just targeted incorrectly?

[michelengelen]

@hmhealey it is indeed targeted incorrectly .. I am changing it to master

[michelengelen]

@hmhealey it is now targeted against master (as it should be) .... I dont know ehy it has been re-targeted against the release branch tbh

[michelengelen]

closing in favor of #8940