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

LPS-119066 [Page Audit] Display the Google PageSpeed issues sections with total count #1005

Closed
wants to merge 12 commits into from
Closed
44 changes: 44 additions & 0 deletions modules/apps/layout/layout-reports-web/jest-setup.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/**
* Copyright (c) 2000-present Liferay, Inc. All rights reserved.
*
* This library is free software; you can redistribute it and/or modify it under
* the terms of the GNU Lesser General Public License as published by the Free
* Software Foundation; either version 2.1 of the License, or (at your option)
* any later version.
*
* This library is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
* FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more
* details.
*/

window.Liferay.Util.sub = function (string = '', data) {
Copy link

Choose a reason for hiding this comment

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

So we have similar logic now in:

I'm making an issue to remove this duplication by baking a reasonable substitute into our default mock environment. Will come back and update the link here once I've done that.

Copy link

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

That config is used to substitute the x in the message keys to check the full text in the tests. It will be awesome to not be required to add it each module. I saw that we are the only team using it in our tests 🤔 layout-reports-web, segments-experiment-web and analytics-reports-web.

const REGEX_SUB = /(?<=-|^)x(?=-|\s)/g;

if (
arguments.length > 2 ||
(typeof data !== 'object' && typeof data !== 'function')
) {
data = Array.prototype.slice.call(arguments, 1);
}

const dataCopy = [...data];
const max = REGEX_SUB.exec(string).length;
Copy link

Choose a reason for hiding this comment

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

Mini-nit: maxmaximum (per guidelines, abbreviations and all that)

Non-blocking feedback though, especially because this file might go away based on liferay/liferay-frontend-projects#519

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the nit. Hopefully, this file will be removed soon and provide it by default in our tests 🍀

let replacedValues = 0;

const replacestring = string.replace
Copy link

Choose a reason for hiding this comment

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

Another non-blocking mini-nit: replacestringreplaceString

Copy link
Author

Choose a reason for hiding this comment

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

Also here...Hopefully, this file will be removed soon and provide it by default in our tests 🍀

? string.replace(REGEX_SUB, () => {
replacedValues = replacedValues + 1;
const lastReplacement = replacedValues >= max;

if (lastReplacement) {
return dataCopy.join('');
}
else {
return dataCopy.shift();
}
})
: string;

return replacestring;
};
8 changes: 8 additions & 0 deletions modules/apps/layout/layout-reports-web/package.json
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
{
"dependencies": {
"@clayui/alert": "3.26.0",
"@clayui/badge": "3.2.0",
"@clayui/button": "3.6.0",
"@clayui/drop-down": "3.25.1",
"@clayui/icon": "3.1.0",
"@clayui/label": "3.4.1",
"@clayui/layout": "3.3.0",
"@clayui/loading-indicator": "3.2.0",
"@clayui/panel": "3.25.0",
"@clayui/progress-bar": "3.2.0",
"@clayui/tooltip": "3.4.5",
"@liferay/frontend-js-react-web": "*",
"classnames": "2.2.6",
Expand All @@ -15,6 +18,11 @@
"react": "16.12.0",
"recharts": "1.8.5"
},
"jest": {
"setupFiles": [
"<rootDir>/jest-setup.config.js"
]
},
Comment on lines +21 to +25
Copy link

Choose a reason for hiding this comment

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

(Outside the scope of this PR, but this is another thing that is more or less repeated verbatim in about 10 places in liferay-portal now. Seems like the kind of thing we might be able to abstract away... Just throwing that out there in case anybody reading this feels like writing up a ticket for it — I'm not totally convinced it's a good idea, so I'm not going to right now).

"name": "layout-reports-web",
"private": true,
"scripts": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import com.liferay.portal.kernel.model.Layout;
import com.liferay.portal.kernel.module.configuration.ConfigurationProvider;
import com.liferay.portal.kernel.portlet.JSONPortletResponseUtil;
import com.liferay.portal.kernel.portlet.LiferayPortletResponse;
import com.liferay.portal.kernel.portlet.bridges.mvc.BaseMVCResourceCommand;
import com.liferay.portal.kernel.portlet.bridges.mvc.MVCResourceCommand;
import com.liferay.portal.kernel.security.permission.PermissionChecker;
Expand All @@ -59,9 +60,11 @@
import java.util.Optional;

import javax.portlet.PortletRequest;
import javax.portlet.PortletResponse;
import javax.portlet.PortletURL;
import javax.portlet.ResourceRequest;
import javax.portlet.ResourceResponse;
import javax.portlet.ResourceURL;

import org.osgi.service.component.annotations.Activate;
import org.osgi.service.component.annotations.Component;
Expand Down Expand Up @@ -117,7 +120,8 @@ protected void doServeResource(
_portal.getPathContext(resourceRequest) + "/assets/"
).put(
"canonicalURLs",
_getCanonicalURLsJSONArray(resourceRequest, layout)
_getCanonicalURLsJSONArray(
resourceRequest, resourceResponse, layout)
).put(
"configureGooglePageSpeedURL",
_getConfigureGooglePageSpeedURL(resourceRequest)
Expand Down Expand Up @@ -158,7 +162,8 @@ private String _getCanonicalURL(
}

private JSONArray _getCanonicalURLsJSONArray(
PortletRequest portletRequest, Layout layout) {
PortletRequest portletRequest, PortletResponse portletResponse,
Layout layout) {

Locale defaultLocale = _getDefaultLocale(layout);

Expand Down Expand Up @@ -208,6 +213,10 @@ private JSONArray _getCanonicalURLsJSONArray(
}
).put(
"languageId", LocaleUtil.toW3cLanguageId(locale)
).put(
"layoutReportsIssuesURL",
_getResourceURL(
layout.getGroupId(), canonicalURL, portletResponse)
).put(
"title", _getTitle(portletRequest, layout, locale)
).build()
Expand Down Expand Up @@ -307,6 +316,21 @@ private Locale _getDefaultLocale(Layout layout) {
}
}

private String _getResourceURL(
long groupId, String canonicalURL, PortletResponse portletResponse) {

LiferayPortletResponse liferayPortletResponse =
_portal.getLiferayPortletResponse(portletResponse);

ResourceURL resourceURL = liferayPortletResponse.createResourceURL();

resourceURL.setParameter("groupId", String.valueOf(groupId));
resourceURL.setParameter("canonicalURL", canonicalURL);
resourceURL.setResourceID("/layout_reports/get_layout_reports_issues");

return resourceURL.toString();
}

private String _getTitle(
PortletRequest portletRequest, Layout layout, Locale locale) {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ private void _processBodyBottomTagBody(PageContext pageContext)

sb.append(iconTag.doTagAsString(pageContext));

sb.append("</div></div></div><div class=\"p-3 sidebar-body\"><span ");
sb.append("</div></div></div><div class=\"sidebar-body\"><span ");
sb.append("aria-hidden=\"true\" class=\"loading-animation ");
sb.append("loading-animation-sm\"></span>");

Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,7 @@
margin-bottom: 0.5rem;
}
}

.dropdown-menu__languages {
max-height: 295px;
}
Comment on lines +57 to +59
Copy link

Choose a reason for hiding this comment

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

Just referencing prior discussion about this pattern from your other pull, in case anybody wants to look.

Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {useEventListener} from '@liferay/frontend-js-react-web';
import React, {useEffect, useState} from 'react';

import LayoutReports from './components/LayoutReports';
import {StoreContextProvider} from './context/StoreContext';

import '../css/main.scss';

Expand Down Expand Up @@ -69,10 +70,13 @@ export default function ({
);

return (
<LayoutReports
eventTriggered={eventTriggered}
isPanelStateOpen={isPanelStateOpen}
layoutReportsDataURL={layoutReportsDataURL}
/>
<StoreContextProvider>
<LayoutReports
eventTriggered={eventTriggered}
isPanelStateOpen={isPanelStateOpen}
layoutReportsDataURL={layoutReportsDataURL}
portletNamespace={portletNamespace}
/>
</StoreContextProvider>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export default function BasicInformation({canonicalURLs, defaultLanguageId}) {
};

return (
<ClayLayout.ContentRow>
<ClayLayout.ContentRow verticalAlign="center">
<ClayLayout.ContentCol>
<div className="inline-item-before">
<ClayLayout.ContentRow>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@
*/

import ClayLink from '@clayui/link';
import PropTypes from 'prop-types';
import React from 'react';
import React, {useContext} from 'react';

import {StoreStateContext} from '../context/StoreContext';

export default function EmptyLayoutReports() {
const {data} = useContext(StoreStateContext);

const {assetsPath, configureGooglePageSpeedURL} = data;

export default function EmptyLayoutReports({
assetsPath,
configureGooglePageSpeedURL,
}) {
const defaultIllustration = `${assetsPath}/issues-default.svg`;

return (
Expand All @@ -28,7 +30,7 @@ export default function EmptyLayoutReports({
alt={Liferay.Language.get(
'default-page-audit-image-alt-description'
)}
className="c-mb-4 c-mt-5"
className="c-my-4"
src={defaultIllustration}
width="120px"
/>
Expand Down Expand Up @@ -68,8 +70,3 @@ export default function EmptyLayoutReports({
</div>
);
}

EmptyLayoutReports.propTypes = {
assetsPath: PropTypes.string.isRequired,
configurePageSpeedURL: PropTypes.string,
};
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ export default function LanguagesDropdown({
<ClayDropDown
active={active}
hasLeftSymbols
menuElementAttrs={{
className: 'dropdown-menu__languages',
}}
onActiveChange={setActive}
trigger={
<ClayButton
Expand Down
Loading