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

Added table of contents for general settings #18404

Merged
merged 7 commits into from Dec 6, 2021

Conversation

bx80
Copy link
Contributor

@bx80 bx80 commented Nov 29, 2021

Description:

Fixes #13837

Adds a table of contents to the general settings page. Anchors and table of content links for plugin sections are automatically added by the controller.

Review

@bx80 bx80 added c: Usability For issues that let users achieve a defined goal more effectively or efficiently. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Nov 29, 2021
@bx80 bx80 added this to the 4.7.0 milestone Nov 29, 2021
@bx80 bx80 self-assigned this Nov 29, 2021
@bx80 bx80 added the Needs Review PRs that need a code review label Dec 1, 2021
Copy link
Contributor

@justinvelluppillai justinvelluppillai left a comment

Choose a reason for hiding this comment

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

I've got this going on

Screen Shot 2021-12-03 at 9 58 21 AM

I can't exactly see why it's line breaking where it is.

Other than this TOC working well and code looks good.

Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

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

FYI I've done a quick test here as well because I saw the screenshot and was wondering if we could maybe somehow shorten some of the titles because they are quite long and it takes quite a bit to read/scan this menu? Like could we rename somehow CONFIGURATION FOR SERVER VARIABLES USED BY GEOIP 2 SERVER to GeoIp

btw the link for geoip did not work for me
image

Also noticed the CORS link is missing
image

I'm also wondering if maybe regular case would be better readable
image

and thinking maybe we have to put a space before capital letters there just to have it bit more human readable? We're doing the same on the Marketplace using something like below but it's in PHP

    function matomoAddSpacesToCamcelCase($name)
    {
        $parts = preg_split('/(?=[A-Z0-9])/', $name);
        $name = '';

        foreach ($parts as $part) {
            if (strlen($part) > 1) {
                $name .= ' ';
            }
            $name .= $part;
        }

        return trim($name);
    }

@bx80
Copy link
Contributor Author

bx80 commented Dec 3, 2021

The text wrapping issue was caused by there not being a suitable white space character between links, they look like they have spaces but it's actually a right margin applied by the materialize classes.

The long plugin names were because I was using the plugin title rather than the plugin name, I've switched to using the plugin name which is shorter, this has also fixed the broken GeoIp2 link.

The 'CORS' and 'Trusted Matomo Hostname' sections both come from the CoreAdminHome plugin, so creating menu items based on the plugin name didn't work, I've added some extra code to handle CoreAdminHome sub sections.

I've also overridden the materialize card uppercase text CSS and added a space before each capital letter for plugin names, which does look a lot better.

Copy link
Contributor

@justinvelluppillai justinvelluppillai left a comment

Choose a reason for hiding this comment

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

Looks good to me. Not sure if we want the tracking code TOC to look the same now (maybe a separate issue anyway)?

Will leave open in case @tsteur has any other feedback.

@bx80 bx80 merged commit 01a7b37 into 4.x-dev Dec 6, 2021
@bx80 bx80 deleted the m-13837-general-settings-toc branch December 6, 2021 20:01
@tsteur
Copy link
Member

tsteur commented Dec 6, 2021

@bx80 fyi just saw there's actually maybe an issue with SEOWebVitals plugin where it makes S E O Web Analytics

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Usability For issues that let users achieve a defined goal more effectively or efficiently. Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Table of Content to General Settings
3 participants