-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Help Center: Align 'X' close button with Help Center icon #94021
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Async-loaded Components (~120 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
@@ -24,6 +24,10 @@ export const calculateOpeningPosition = ( element: HTMLElement ) => { | |||
return defaultPosition; | |||
} | |||
|
|||
// This takes into account the '?' button's padding and align the close button with the help center icon | |||
const computedStyle = window.getComputedStyle( element ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, this call is reallly slow. It will typically force style recalc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey 👋 Thank you for the tip. I know it can be relatively slow. Since this happens only when opening help center and is not part of a loop or anything like that, I think this is fine.
Are you concerned about it? I could try other potential solutions, but I am afraid it might bring more complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It happens on every click anywhere on the page when the Help Center is closed :(
It will make every button click slower.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have pushed a new commit that relies on getComputedStyles
just once, subsequently, it relies on data-attributes
which is faster than getComputedStyles
.
I am happy with this approach, but you are welcome to propose another idea :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's beautiful! Testing now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The caching idea is perfect! Left some cosmetic comments and found a tiny issue in RTL.
const helpCenterPaddingRight = parseInt( computedStyle.paddingRight, 10 ); | ||
const helpCenterIconPaddingRight = | ||
parseInt( element.style.paddingRight, 10 ) || | ||
parseInt( element.getAttribute( 'data-padding-right' ) || '0', 10 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do element.dataset.paddingRight
btw. It automatically covnerts kebab case to camel case.
@@ -36,7 +37,7 @@ export const calculateOpeningPosition = ( element: HTMLElement ) => { | |||
const { x, y, width, height } = element.getBoundingClientRect(); | |||
|
|||
const buttonLeftEdge = x; | |||
const buttonRightEdge = x + width + helpCenterPaddingRight; | |||
const buttonRightEdge = x + width + helpCenterIconPaddingRight; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we need to make this - helpCenterIconPaddingRight
in RTL?
@@ -24,6 +24,10 @@ export const calculateOpeningPosition = ( element: HTMLElement ) => { | |||
return defaultPosition; | |||
} | |||
|
|||
// This takes into account the '?' button's padding and align the close button with the help center icon | |||
const computedStyle = window.getComputedStyle( element ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's beautiful! Testing now.
// Cache the computed padding-right if not already done | ||
// This will be used to align the help center close button with the "?" icon | ||
if ( ! openingElement.hasAttribute( 'data-padding-right' ) ) { | ||
const computedStyle = window.getComputedStyle( openingElement ); | ||
openingElement.setAttribute( 'data-padding-right', computedStyle.paddingRight ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should happen inside calculateOpeningPosition
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can move it there, still works fine.
I am, honestly, not sure where is the best place to put this logic. Would you please elaborate a bit more why you believe inside the function is a better place?
Moved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this value is used in calculateOpeningPosition
and useOpeningCoordinates
doesn't need it.
After discussing p1724940421518469/1724923219.497729-slack-C02T4NVL4JJ with team vertex, I have decided to change the approach to addressing this issue, new PR |
Related to https://github.com/Automattic/dotcom-forge/issues/8829
Fixes https://github.com/Automattic/dotcom-forge/issues/8829
Proposed Changes
Why are these changes being made?
Testing Instructions
my home
, it should be positioned as beforePre-merge Checklist