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

feat: add setting to only show icons on small screens #54

Merged
merged 2 commits into from
Feb 7, 2023
Merged

feat: add setting to only show icons on small screens #54

merged 2 commits into from
Feb 7, 2023

Conversation

SychO9
Copy link
Contributor

@SychO9 SychO9 commented Feb 1, 2023

Changes proposed in this pull request:
Links can overflow the header of the app, this PR adds a setting to only show icons on smaller screens.

Reviewers should focus on:

  • Is this fine to have in the extension?

Screenshot
Screenshot from 2023-02-01 13-03-00

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

Required changes:

@imorland imorland linked an issue Feb 1, 2023 that may be closed by this pull request
@@ -68,6 +68,20 @@
width: auto;
margin-right: 10px;
}

@media (max-width: 1100px) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be easier to wrap both blocks with the when clause rather than two separate ones?

@imorland imorland merged commit cef36cf into FriendsOfFlarum:master Feb 7, 2023
@SychO9 SychO9 deleted the sm/improve-html-label-structure-for-customization branch February 7, 2023 08:50
@iPurpl3x
Copy link
Contributor

iPurpl3x commented Mar 29, 2023

Hi @SychO9,

I have tried this update and I have encountered an error on one of my instances:

variable @fof-links-show-only-icons-on-mobile is undefined in file /opt/flarum/vendor/fof/links/less/forum.less in forum.less on line 74, column 15

On a clean installation without any other extensions, it seems to work, so what could be the cause of this?... I have checked and there are no other installed extensions that use the registerLessConfigVar() extender method.

Then I also realized that on small mobile devices (phone breakpoint) the icon is also hidden, even though the links are in the burger menu, and there it doesn't make sense to hide the label IMO.

image

Finally, I find the settings name quite confusing, it would be much simpler to name it like this: "hide link label on tablet" (not "on mobile", as mentioned above.

@SychO9
Copy link
Contributor Author

SychO9 commented Mar 29, 2023

For the label and the icons issue: #57 (thanks for the report)

For the first issue, it's hard to tell what could be wrong, it could be a type of extension that overrides certain core components. I would try disabling the most advanced extensions one at a time and see.

@iPurpl3x
Copy link
Contributor

iPurpl3x commented Apr 4, 2023

Yeah, I now found the reason and could fix it: I was overriding the flarum.assets.factory. I could extend it instead of overriding it, that fixed the issue of the variable being undefined.

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

Successfully merging this pull request may close these issues.

Bad visual top bar on some screen size
4 participants