Implement mobile tabbed view for Report Table and Popular Pages Widget. #9765
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Build files for c8b866e are ready:
|
|
Size Change: +1.27 kB (+0.07%) Total Size: 1.89 MB
ℹ️ View Unchanged
|
techanvil
commented
Nov 26, 2024
| @@ -31,29 +31,17 @@ | |||
| border-radius: $br-sm; | |||
|
|
|||
| .googlesitekit-widget-audience-tiles__tabs { | |||
| border-bottom: 1px solid $c-utility-divider; | |||
There was a problem hiding this comment.
The tabs in the new design use the same styling as the Audience Tiles widget's tabs, so the common style has been extracted for reuse in the googlesitekit-tab-bar--start-aligned-high-contrast class below.
techanvil
commented
Nov 26, 2024
| @@ -52,174 +59,221 @@ export default function ReportTable( props ) { | |||
| 'limit must be an integer, if provided.' | |||
| ); | |||
|
|
|||
| const mobileColumns = columns.filter( ( col ) => ! col.hideOnMobile ); | |||
| function isHiddenOnMobile( hideOnMobile ) { | |||
There was a problem hiding this comment.
The hideOnMobile column flag is not relevant for the tabbed layout.
In fact, the ModulePopularPagesWidgetGA4 widget was the only widget making use of the hideOnMobile flag so we could go one further and completely strip it out of this component.
If so, I'd suggest we do so in a followup issue rather than this one which has hit the estimate in execution.
techanvil
commented
Nov 26, 2024
| @@ -49,6 +49,7 @@ export default function DetailsPermaLinks( { title, path, serviceURL } ) { | |||
| return ( | |||
| <Fragment> | |||
| <Link | |||
| className="googlesitekit-font-weight-medium" | |||
There was a problem hiding this comment.
I've gone ahead and set the medium font weight here so it's applied to the desktop viewport and WP dashboard "top content" widget as well, as per this Slack thread, as I think it's most likely that we'll go ahead with this approach.
If it turns out we decide not to, it will be quick to amend in a followup issue.
There was a problem hiding this comment.
Update: This approach has now been agreed.
techanvil
commented
Nov 26, 2024
| export const ReportTableBasic = Template.bind( {} ); | ||
| ReportTableBasic.storyName = 'Basic'; | ||
| ReportTableBasic.args = createBasicArgs(); | ||
| ReportTableBasic.scenario = {}; |
There was a problem hiding this comment.
It didn't make sense to add a VRT scenario for the ReportTableTabbedLayout story without adding one for the basic version.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist