Comments (17)
Noting that additional minor tweaks were made to the tiles so they would be able to contain the badge in the bottom:
- Tiles height was increased to 150 px
- String values (like top traffic source) font was changes to “title medium”
- The spacing between the value and the text below (i.e “of x visitors) was reduced
- In tiles that contains lists (like top pages, keywords etc.) the spacing between the list and the tile title was reduced by 3 pixels to align with the other tiles
- Note that in the visit length value new format the “m” and the “s” are in smaller fonts then the numbers
Figma is updated with the new tiles layout, You can find it here
If needed for comparison, old tiles layout (current one) can be found here
from site-kit-wp.
Thanks both, definitively worth checking but as @zutigrm has pointed out, the mobile tile height is intended to be derived from the content, so it's OK for tile heights to differ on mobile viewports.
Admittedly, the height is out by a few pixels from the current design (see below), but as Aleksej also mentioned we've got an update to the design coming in #7763, which will affect the tile heights, so I think it's safe to approve this as it is and take a fresh look in the QA for 7763.
from site-kit-wp.
Thanks @techanvil
Moving it back with you @wpdarren
from site-kit-wp.
Back to you for another look, @wpdarren.
from site-kit-wp.
AC ✅
5. Note that in the visit length value new format the “m” and the “s” are in smaller fonts then the numbers
@sigal-teller GA shows the abbreviated units using the same font size and face (it's all one value) which is how we do it as well. Can we keep it this way?
from site-kit-wp.
Hi @zutigrm, thanks for this IB. It's generally looking good, albeit a bit of a "PR in text form". Sometimes it's appropriate to be very explicit, though. The only real concern I have is around the duplication of .googlesitekit-km-widget-tile__metric-change-container
within MetricTileNumeric
.
By moving the change badge out of the current .googlesitekit-km-widget-tile__metric-change-container
it means that current element is no longer a "change container" and more of a "metric container". Note how it would compare to MetricTileText
where the metric and change badge are rendered separately but there's only one use of .googlesitekit-km-widget-tile__metric-change-container
.
As an aside, drilling into the styling of .googlesitekit-km-widget-tile__metric-change-container
it's evident that some of its styling will no longer be needed - gap
for sure. It's not entirely clear the align-items
would still have an effect either. This sort of thing would hopefully be addressed at implementation/review time anyway.
Of course, we don't need or want to get right into every last detail in the IB, but to my main point, I do think it's worth reconsidering the suggested use of .googlesitekit-km-widget-tile__metric-change-container
to avoid a bit of semantic drift here. Wrapping the ChangeBadge
in this class does make sense but retaining its use on the current element would not be so appropriate.
from site-kit-wp.
@techanvil thank you for the feedback. That is a very good point, thanks. I updated the IB
from site-kit-wp.
Thanks for the rewrite, that looks great! IB LGTM. ✅
from site-kit-wp.
QA Update: ❌
@zutigrm on smaller viewports, i.e. mobile, the height of the Most engaged traffic source
and Top traffic source
metric tiles are different than the other numeric and text type key metric tiles. See screenshots below. In the example, for iPhone 12 Pro, the other tiles are 358 x 109.91
, but the height is different on these two tiles.
from site-kit-wp.
@wpdarren Thanks for checking this.
I am not sure height
of the tile is part of the scope, as per recent discussion height
on mobile is supposed to be fluid, and there is this issue #7763 that is going to address some edits in design (and update the current design a bit). Changes we make to the height
as part of this issue will most likely be overriden in the mentioned issue.
@techanvil Let me know what is your opinion here, as you are familiar with both issues as well
from site-kit-wp.
Thanks @zutigrm
I am happy to go with what you think is best. The reason why I queried it was because of this section in the AC:
It should be ensured that the increased height of the metric tiles due to the above changes is consistently reflected across all metric tiles.
Admittedly, it doesn't mention smaller viewports and all looks fine on desktop.
from site-kit-wp.
@wpdarren Yeah the the new issue details come after the AC is written for this one, as there is also new design added by Sigal just few days ago. Let's see what @techanvil thinks is the best approach here
from site-kit-wp.
QA Update: ✅
Verified:
- The metric change badge in numeric and text type key metric tiles are relocated to the bottom of the tile according to the Figma design.
- The typography of & spacing between different elements of the metric tiles are consistent with the Figma design.
- The increased height of the metric tiles due to the above changes is consistently reflected across all metric tiles.
Note: I observed an issue with the height of the tiles on smaller viewports, and this should be improved in 7763.
from site-kit-wp.
I can reproduce a broken layout for the "numeric" tiles when there is another tile with an error state present in the KMW area. Please see this screenshot:
It looks like this is a regression from this issue, specifically from this change.
from site-kit-wp.
QA Update: ❌
Thanks, @nfmohit. I forgot to include error states in my testing. Good spot. I have moved this back to Execution.
from site-kit-wp.
I have reverted the height which was there to align the content to the bottom of the tile. Since this was causing gaps when error tile is present. And error tile height, and content alignment in tiles is handled in other issues. For background, refer to this thread
from site-kit-wp.
QA Update: ✅
Verified:
- I retested this to ensure that the percentage badges continued to appear underneath as per Figam designs but I also tested with different styles of tiles, including an error tile and a non-numeric.
from site-kit-wp.
Related Issues (20)
- Creation of `Top cities driving purchases` ACR KMW
- Creation of `Top cities driving purchases` ACR KMW
- Creation of `Top device driving purchases` ACR KMW
- Creation of `Top device driving purchases` ACR KMW
- Creation of `Top traffic source driving purchases` ACR KMW
- Glitches / errors when changing the audience selection in mobile viewports
- Impossible to stop tagmanager code placement HOT 1
- Agency with hundreds of client Google Analytics accounts unable to assign correct profiles HOT 2
- Remove `conversionInfra` feature flag
- Remove localization for module homepage URLs
- Improve the `resetPublications` action HOT 6
- Requests made for module settings on dashboard
- Implement Internal Measurement/Tracking Following ACR KMW Updates
- Enhance compatibility with Docker locally for E2E
- Refactor the `ZeroDataNotification` to use the new lighter `Notification` component. HOT 2
- Release 1.134.0
- Add "Edit in Reader Revenue Manager" link for the RRM module
- Sort Storybook menu items HOT 1
- Enhance feature activation for BC
- Revert the Fix For VRT failures
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from site-kit-wp.