-
Notifications
You must be signed in to change notification settings - Fork 178
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
Background element refactor (+ drop targets and fixes) #693
Conversation
Size Change: +118 B (0%) Total Size: 433 kB
ℹ️ View Unchanged
|
* master: Background element refactor (+ drop targets and fixes) (#693)
if (!currentPage || currentPage?.backgroundElementId) { | ||
return; | ||
} | ||
const element = createNewElement('shape', { |
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.
If we update the model itself, I think it'd work better in the story reducer rather than a disconnected effect. For one, we wouldn't have history problems with this approach. This might be an important thing to fix right away.
@@ -57,6 +57,7 @@ const Lasso = styled.div` | |||
function SelectionCanvas({ children }) { | |||
const { | |||
actions: { clearSelection }, | |||
state: { selectedElements }, |
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.
Can we use selectedElementIds
instead? They are updating a lot less often.
function Layer({ layer }) { | ||
const { LayerIcon, LayerContent } = getDefinitionForType(layer.type); | ||
const { isSelected, handleClick } = useLayerSelection(layer); | ||
const { handleStartReordering } = useLayerReordering(layer); | ||
const { |
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.
Hmm. I'm seeing the background showing up in the layer panel now, even when not specified. Can you clarify if this some additional issue fixed?
@@ -87,7 +88,9 @@ function FrameElement({ element }) { | |||
if (!isSelected) { | |||
handleSelectElement(id, evt); | |||
} | |||
evt.stopPropagation(); | |||
if (!isBackground) { |
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.
Why is this? Could you add a comment in code. This seems like an important behavior involving background elements. As such, I'm also wondering if it'd be safer to roll it all inside the handleSelectElement
? Otherwise we're fragmenting the behavior a bit between two places. It used to be easier with a simple stopPropagation
, but this could be more nuanced.
actions: { arrangeElement, setSelectedElementsById }, | ||
} = useStory(); | ||
|
||
const isBackground = currentPage.backgroundElementId === elementId; |
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.
Don't we already have element.isBackground
?
[updateCurrentPageProperties] | ||
); | ||
return ( | ||
<SimplePanel name="pagebackground" title={__('Page', 'web-stories')}> |
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 don't need the page background color anymore? Is there a semantical meaning in the background color? Is there technical meaning? E.g. if an image is used as the background and has some transparency, shouldn't the background color show through?
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 checked and it appears to be so in Figma. The page background color is customizable even if the background media is set.
const backgroundColor = getCommonValue(selectedElements, 'backgroundColor'); | ||
|
||
return ( | ||
<SimplePanel name="style" title={__('Style', 'web-stories')}> |
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 need to confirm whether a shape is allowed to be a background per our rules: i.e. can we put a simple shape (rect or star) and click "Use as background" on it. If we do, we should probably disallow "Remove as background" for auto-background-shape. Otherwise it'd simply be recreated immediately.
Confirming further, would we allow isFullbleed customization for the auto-background-shape? If not, does this PR already disable it?
<Color | ||
hasGradient | ||
value={backgroundColor} | ||
isMultiple={backgroundColor === ''} |
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.
There's no longer isMultiple
<SimplePanel name="style" title={__('Style', 'web-stories')}> | ||
<Row> | ||
<Color | ||
hasGradient |
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.
So, there might be a difference here slightly with page background color. See #739. It looks like we may want to remove opacity from the background shape, but not a non-background shape.
@@ -36,12 +36,14 @@ function FlipControls({ value, onChange }) { | |||
<> | |||
<Toggle | |||
title={__('Flip horizontally', 'web-stories')} | |||
aria-label={__('Flip horizontally', 'web-stories')} |
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 could probably default title
for aria-label
as well?
@wassgha Was this migration script somehow never added? 🤔 |
Closes #25
Closes #683
Closes #644
Closes #631
Maybe closes #678 ?
Closes #739
This rethinks the background element as a first class citizen. The background element is either non-existent or one of the supported element types (shape, image, video), it is no longer given a preferential treatment in code.
Changes