Comments (10)
@techanvil Placing it below the widget LGTM (if we'll have the insight notice it will be placed below like it was mentioned here). In KMW it should also appear below the widget, per Figma design. Thanks!
from site-kit-wp.
Nice work on the IB here, @benbowler! Please look at my comments below:
- Add the 783px breakpoint to
assets/js/hooks/useBreakpoint.js
asBREAKPOINT_WP_ADMIN_BAR_TABLET = 'wpadminbar'
I'm not sure we want to do this. For example, currently for a viewport of 784 pixels, useBreakpoint
returns BREAKPOINT_TABLET
which is what a lot of the usages of this function would expect. With the proposed change, the function would return BREAKPOINT_WP_ADMIN_BAR_TABLET
for a viewport of 784 pixels, thus potentially breaking the existing usages.
Instead, we could do something like what we're doing in assets/js/components/FeatureToursDesktop.js
and directly use useWindowWidth
as an exception. What do you think?
- Use the
useBreakpoint
hook, if the breakpoint is! BREAKPOINT_TABLET && ! BREAKPOINT_SMALL
, do not render the CTA component in the widget area header.
If we do go with directly using useWindowWidth
as explained above, let's simplify this to just render the CTA component in the widget area header if the window width is 783 pixels or more.
- If the breakpoint is
BREAKPOINT_TABLET || BREAKPOINT_SMALL
, render the CTA within a newCell
in the footerRow
component. Update the conditional block to make sure this row renders regardless of if there is a Footer set but to only render the Footer if it exists.
If we do go with directly using useWindowWidth
as explained above, let's simplify this to just render the CTA component in the widget area footer if the window width is 782 pixels or less.
- Update
assets/sass/widgets/_widget-area.scss
, add a new media query up to 600px using$bp-mobileOnly
:
Let's propose a mobile-first approach here, so that the suggested styles are added as default, and overridden on viewports with a min-width of $bp-nonMobile
.
Please let me know what you think, thank you!
from site-kit-wp.
Noting as per communication with @techanvil that the implementation of this ticket needed a bit more effort and action than the IB stated, mostly due to:
- There could not simply just be a new cell added to the footer output, this resulted in the CTA and existing footer elements sitting on different horizontal planes
- I had to introduce dynamic cell sizing based on large, medium and small viewports
- This poses a risk that if the existing footer elements are widget than the current simple "source" element, it may still push footer and CTA to separate horizontal planes, we may need to consider this in future if we introduce longer footer elements
- The new badge needed to have a fixed height set, as if not set it would adopt the height of the subtitle/subheader area (due to flex) and it would look more like a circle. Providing a fixed height takes care of this
I will update the IB based on this, cc @benbowler @techanvil
from site-kit-wp.
This is something that I think needs a little confirmation from @sigal-teller and @techanvil. The CTA button here is attached to the widget area. We can make it show up in the footer of the widget area for mobile breakpoints (this will also apply globally and will impact the KM "Change metrics" CTA as well), but the Audience Segmentation widget area also has an additional InfoNoticeWidget underneath the audience tiles, so the "Change groups" CTA will appear below the InfoNoticeWidget. Is it okay to show it there, or do we want to do something else here?
Here's a mock of what it may look:
from site-kit-wp.
Thanks @kelvinballoo and @nfmohit.
Showing the CTA below the widget area as per the mockup above looks fine to me. We do have a design in Figma showing the CTA on the same line as the source link:
Applying it across the board, i.e. to the KM widget area as well also makes sense - here's how it currently looks at a narrow mobile viewport:
Here's a mockup showing how it would look with the CTA below the widget area:
As I've said it's all LGTM, but it would be good to get @sigal-teller's thoughts too.
from site-kit-wp.
Thanks @sigal-teller.
Just noting that, as per the Figma design, we'll show the CTA below the widget area up to a breakpoint of 782px, from 783px we'll show it above the widget area (see related comment thread).
from site-kit-wp.
Noting that as discussed on Figma, we'll include the alignment of the "New" badge in this issue as well as the widget area CTA.
from site-kit-wp.
IB ✅ Thanks @benbowler !
from site-kit-wp.
Also noting that due to eslint complexity violations (exceeded by 6 levels of complexity), resulting efforts included the separation of the new badge component into it's own WidgetNewBadge component, accepting a widget area slug as the only parameter. This will be detailed in the IB following updates.
from site-kit-wp.
QA Update ✅
- Tested on dev environment.
- Verified CTA and NEW badge position is now as per Figma and AC in mobile, tablet and Desktop.
- Tested on mobile, tablet and desktop viewports.
- Tested on 600px, 601px, 783px, 960px and 961px and above.
from site-kit-wp.
Related Issues (20)
- Enhance feature activation for BC
- Revert the Fix For VRT failures HOT 1
- Google Site Kit plugin fails to load data on my WordPress website HOT 1
- Fix issue where VRT test fails in CI when launching Chromium HOT 1
- Ensure cached audiences are resynced when the No Audiences Banner is shown HOT 1
- Enhancement for subtle notifications
- Enhancement on CTA order on mobile for subtle notifications
- Creation of `Top traffic source driving leads` ACR KMW HOT 1
- Ensure the "Top content" CTA for creating the custom dimension appears when the corresponding report error shows the custom dimension is missing
- Storybook error during build: `Error: Source and destination must not be the same.` HOT 2
- Refactor the `AudienceTiles` component to make it easier to read
- Gathering data notification - Loader not appearing on 'See other services' CTA click
- 'a.getTime is not a function' error on dashboard
- Zero Data Notification Not Appearing consistently - on First Load in New Tab/Window HOT 5
- Extract the audience utility logic from `Analytics_4` to a new class.
- Fix glitches in the Audience Segmentation Setup CTA Banner HOT 1
- "Looks like your site is not allowed access to Google account data" error, despite no changes made HOT 3
- PAX application not loading correctly on the Ads setup screen. HOT 2
- Column Titles Overlapping in 'Top Content Over the Last X Days' Widget on Narrow Viewports.
- Release 1.135.0
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.