Automattic / newspack-popups

Popup notifications.
GNU General Public License v2.0
70 stars 17 forks source link

fix: update no-padding popup styles #1282

Open laurelfulford opened 4 months ago

laurelfulford commented 4 months ago

All Submissions:

Changes proposed in this Pull Request:

Right now, the Default and Hide Padding styles for the overlay prompts look the same. This PR makes the 'No Padding' style a true no padding style, but it's worth flagging that prior to this, that style has always had some padding.

I think it's naming/styling came from trying to be a popup variation of the inline prompt's no border style, but unlike removing the border and padding from the inline prompts, an overlay prompt with no padding and no blocks inside with padding looks broken:

image

No padding also adds a lot of flexibility to the designs, but I'd appreciate some feedback around whether this seems like an okay trade off:

image

If we do opt to go with this, we should probably present it as something of a breaking change, as it'll change the appearance of this style in ways that may not work with live prompts that are using it.

See 1200550061930446-as-1206132801968396.

How to test the changes in this Pull Request:

  1. Create a new campaign prompt, and set it to display as an overlay. Give it some regular content.
  2. Under the Styles panel on the right, set the prompt to have 'Hide Padding'.
  3. Preview and note that the overlay popup has padding:

image

  1. Apply the PR and run npm run build.
  2. Refresh the preview and confirm that the popup now has no padding:

image

  1. Switch out the content blocks to add something a little more logical -- for example, a Cover Block, Media & Text Block, or Group block + padding as a base. Preview again and confirm this works well, and that there truly is no padding:

image

  1. Test on smaller screen sizes, too, and make sure things look okay.
  2. As part of this PR, I also added some styles to make sure the overlay always contrasts against the overlay when using the 'No Padding' style. To test this, try to change the overlay colour from a dark to a light colour, and confirm that the close button contrasts against it; test this with a few different colours:

image

  1. Test this with the 'No Overlay' option, and confirm that the close button is black.
  2. Switch the overlay back on and back to a dark colour, and test the Default and Large Styles for the overlay; confirm that the close button is black for both, even though it would be white for the Hide Padding style.
  3. Add a featured image to the prompt; confirm that the button colour remains white, even when the overlay is light:

image

Other information:

adekbadek commented 4 months ago

I think the offsetting of both the preview label and the close button are an issue now:

image image

@thomasguillot / @kevinzweerink – could you weight in here regarding the "no padding" styles? We need to find suitable placement for these two elements with the "no padding" style.

thomasguillot commented 4 months ago

In https://github.com/Automattic/newspack-popups/pull/1282/commits/e7f1fa78727db90a7aaeefe183afdf3a536ffc7f I slightly tweaked the position of the "Preview" label

Screenshot 2024-03-07 at 10 32 31@2x

thomasguillot commented 4 months ago

For the close button, the position doesn't bother me as much. But I wonder what @kevinzweerink thinks about this.

kevinzweerink commented 4 months ago

Close button looks good to me! @thomasguillot 's change to the preview label looks right too.