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

Anchor the appsbar icon to SiteURL #347

Merged
merged 1 commit into from
Jan 10, 2023
Merged

Conversation

lieut-data
Copy link
Member

Summary

This ensures the icon can be correctly loaded when a subpath is configured and the application isn't running at the root.

Ticket Link

Fixes: #343

This ensures the icon can be correctly loaded when a subpath is configured and the application isn't running at the root.

Fixes: #343
@lieut-data lieut-data added the 2: Dev Review Requires review by a core committer label Dec 12, 2022
@codecov
Copy link

codecov bot commented Dec 12, 2022

Codecov Report

Base: 33.32% // Head: 33.32% // No change to project coverage 👍

Coverage data is based on head (9eccd41) compared to base (09907ab).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #347   +/-   ##
=======================================
  Coverage   33.32%   33.32%           
=======================================
  Files          21       21           
  Lines        2785     2785           
=======================================
  Hits          928      928           
  Misses       1748     1748           
  Partials      109      109           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@hanzei hanzei added this to the v1.6.0 milestone Dec 12, 2022
@hanzei hanzei added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core committer labels Dec 13, 2022
@DHaussermann
Copy link

@lieut-data for this PR I do not see the icon working on our subpath test server.
image
The URL is set to https://subpath.test.mattermost.com/mattermost as expected

My first though is that this solution is dependant on your server side code here mattermost/mattermost#21886 is this correct?

If so, I can't test this PR right away as I have no reliable way of getting the server side code change onto a subpath setup right now.

That being said I did regression test this change on other servers and see no issue with the App Bar icon display.

Do you have any thoughts on merging this now to include in the next release?

@lieut-data
Copy link
Member Author

@DHaussermann, yes this PR depends on mattermost/mattermost#21886. Assuming it doesn't regress as you've identified for non-subpath servers, I'm 👍 for merging now.

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed

  • Regression testing was done on this PR with no impacts found.
  • Plugin icons for App Bar on sub-path servers will not be supported until Mattermost v7.8.0
  • Once I have the infrastructure in place, I will test and create a new issue if any further changes are needed to support App Bar icons on sub-path servers for GitLab

LGTM!
Thanks @lieut-data for the effort on this and the broader solution.

Please merge.

@lieut-data lieut-data merged commit b45b46e into master Jan 10, 2023
@lieut-data lieut-data deleted the issue-343-subpath-icon branch January 10, 2023 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: QA Review Requires review by a QA tester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subpath server is missing GitLab icons on Upgrade
5 participants