-
Notifications
You must be signed in to change notification settings - Fork 295
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
PoC: Set a page content max-width for the plugin and settings. #10322
base: develop
Are you sure you want to change the base?
Conversation
@media (min-width: $page-content-max-width) { | ||
// Once the page content reaches it's max-width, fix the overlay notifictation to the | ||
// right had side of the page content. | ||
right: calc((100vw - #{$page-content-max-width}) / 2); // NOTE: this doesn't account for the WP menu collapsed state. |
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.
As noted here, over the content max width the overlay notification position does not account for the collapsed state of the WordPress menu. This means that the notifications distance from the right of the page content column changes when opening and closes the menu.
We can address this in the final ticket using an approach similar to the user input app. I will add this to the main IB to be implemented there.
const [ showSurveyPortal, setShowSurveyPortal ] = useState( false ); | ||
const currentSurvey = useSelect( ( select ) => | ||
select( CORE_SITE ).isUsingProxy() && | ||
select( CORE_USER ).areSurveysOnCooldown() === false | ||
? select( CORE_USER ).getCurrentSurvey() | ||
: null | ||
); |
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.
We should short-circuit this useSelect()
callback to avoid making a potentially unneeded request to the survey
endpoint on page load.
Plus, rename the state variables as it's no longer rendering the portal.
const [ showSurveyPortal, setShowSurveyPortal ] = useState( false ); | |
const currentSurvey = useSelect( ( select ) => | |
select( CORE_SITE ).isUsingProxy() && | |
select( CORE_USER ).areSurveysOnCooldown() === false | |
? select( CORE_USER ).getCurrentSurvey() | |
: null | |
); | |
const [ showSurvey, setShowSurvey ] = useState( false ); | |
const showCurrentSurvey = useSelect( ( select ) => | |
showSurvey && | |
select( CORE_SITE ).isUsingProxy() && | |
select( CORE_USER ).areSurveysOnCooldown() === false | |
? select( CORE_USER ).getCurrentSurvey() | |
: null | |
); |
@@ -43,6 +43,12 @@ | |||
} | |||
} | |||
|
|||
@media (min-width: $page-content-max-width) { |
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.
Arguably this rule should kick in from min-width: $page-content-max-width + 1
, but in practical terms it won't make a difference to leave as-is...
Summary
Addresses issue:
Relevant technical choices
PoC PR, requires additional work such as accounting for the WP menu to get over the line.
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