Skip to content
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

Blocks > Creating a new Page block should have a block template with a background image layer (populated with a default image) and a vertical grid layer #1531

Closed
4 tasks done
alcurrie opened this issue Oct 24, 2018 · 10 comments
Labels
Enhancement New feature or improvement of an existing one
Milestone

Comments

@alcurrie
Copy link

alcurrie commented Oct 24, 2018

As an author creating a new AMP story page, I want the default block template to have a background image layer and a vertical grid layer. The background layer should be populated with a default image

  • AC 1: When creating a new AMP story page the default template will include the following elements:
    • A background image layer block populated with a default image
    • A vertical grid layer on top of the background image layer
  • AC 2: AMP story author can add/update/change the image in the background from the default image to another background image of their choosing.
    Note: AC was updated to not require background image as based on
@alcurrie alcurrie self-assigned this Oct 24, 2018
@alcurrie alcurrie removed their assignment Nov 1, 2018
@alcurrie alcurrie added the UX label Nov 1, 2018
@jwold
Copy link
Collaborator

jwold commented Nov 1, 2018

This one is not blocking development, UX just needs to provide a default image and an icon.

[Edit: Suggestion for icon can be found here]

@miina miina added the BE label Nov 1, 2018
@miina miina self-assigned this Nov 7, 2018
@jwold
Copy link
Collaborator

jwold commented Nov 8, 2018

Are we suggesting a placeholder video for video fill layer as well? Wondering if the placeholders don't quite make sense.

@miina
Copy link
Contributor

miina commented Nov 8, 2018

@jwold Just saw this now, as chatted as well then probably having a default image / video wouldn't be the most user-friendly option and perhaps just expanding the default media selector to fill in the whole layer would make more sense at this moment. That should be quite straightforward to implement as well and if we see that in practice it doesn't seem to be a good idea then we could think of an alternative. Thoughts?

cc @alcurrie

@jwold
Copy link
Collaborator

jwold commented Nov 8, 2018

@miina agreed. I think we should just expand the area and see how that "feels" when we get it live. Then probably go forward with that for now.

Thanks!

@jwold jwold removed the UX label Nov 8, 2018
@miina miina added the FE label Nov 12, 2018
@miina miina assigned mehigh and unassigned miina Nov 19, 2018
@mehigh
Copy link
Contributor

mehigh commented Nov 19, 2018

Selecting a layer beneath another is still not possible.
Current state of things: https://dsh.re/59b6e

I’ve spent 2 focused sessions on this ticket today and not much to show for :/ the animations / blurring are all UX improvements, but not being able to choose an image to upload in the lower layer is something still at play.

There is a core click listener which changes the is-selected element as soon as you start clicking (it's on mouse down, not even on mouse up).. and it's bringing the next sibling in the DOM into focus - without the is-selected class until you click again though.

The click handler comes from React-DOM:
http://cloud.urldocs.com/99fadbb90457

I'd like to bounce this off with someone else as I can't seem to get past beyond this blocker. :/

@cathibosco
Copy link

In our walk through and workshop sessions: We were discussing this:

  • if you are working on a layer, then every layer that is above it would not be visible or actionable -
    and every layer below it would be blurred and cut to 30% opacity.
    It looks like from the video you are not experiencing that when working on a fill layer. We were working with the block navigator though...
    Dropping this mock up in case it is helpful. Gosh I wish I could help more.

overall-scene-7e

@mehigh
Copy link
Contributor

mehigh commented Nov 20, 2018

@miina
The last thing - if we can keep the is-selected class of the element we have in focus after we start clicking in the amp story all our problems should be gone:
https://dsh.re/2de3e
If we have to use some other class than is-selected, so be it.. in order to differentiate ourselves from the core Gutenberg functionality which ads and removes classes at will?

@mehigh mehigh assigned alcurrie and unassigned mehigh and miina Nov 20, 2018
@mehigh
Copy link
Contributor

mehigh commented Nov 20, 2018

Just merged it in, should be deployed soon.

@miina
Copy link
Contributor

miina commented Nov 20, 2018

Closing as fixed in #1600.

@miina miina closed this as completed Nov 20, 2018
@alcurrie
Copy link
Author

I've confirmed adding a new page opens with a default template will include the following elements:

  • A background image layer block
  • A vertical grid layer on top of the background image layer
  • media loader displays by default on the page and the AMP story author can add an image in the background and by default the image fills the space
    See screencast: https://cl.ly/8ea95bd1178e

As discussed above, original AC was updated per discussion here: (#1531 (comment)) we recommended not including a default image and instead expanding the default media selector to fill in the whole layer.

@alcurrie alcurrie removed their assignment Dec 3, 2018
@westonruter westonruter added this to the v1.2 milestone May 21, 2019
@swissspidy swissspidy added Enhancement New feature or improvement of an existing one and removed AMP-Stories-Extension labels Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or improvement of an existing one
Projects
None yet
Development

No branches or pull requests

7 participants