-
Notifications
You must be signed in to change notification settings - Fork 13
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
Attribute standardization #1685
Conversation
@copilot-robot prerelease |
Hi, @tim-evans, a pre-release has been published:
|
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.
Everything in this PR looks OK, however I am a little confused with whatever is happening with captions?
isAutogenerated?: boolean; | ||
linkStyle?: "button"; |
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 is no longer needed?
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 was a legacy property that was replaced by a button annotation.
@@ -379,102 +379,4 @@ describe("ReactRenderer", () => { | |||
); | |||
}); | |||
}); | |||
|
|||
describe("Subdocuments", () => { |
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 another test for the caption related changes? Looks like the other tests have caption related things?
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.
Subdocuments are deprecated in favor of slices and we haven't used them for many months at this point.
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.
LGTM
Please let me merge this, as it will cause cascading changes upstream. |
8b01760
to
448caed
Compare
@copilot-robot prerelease |
Hi, @tim-evans, a pre-release has been published:
|
@copilot-robot prerelease |
Hi, @tim-evans, a pre-release has been published:
|
bb88415
to
fde3165
Compare
@copilot-robot prerelease |
Hi, @tim-evans, a pre-release has been published:
|
fde3165
to
ee65f5d
Compare
@copilot-robot prerelease |
Hi, @tim-evans, a pre-release has been published:
|
@copilot-robot prerelease |
Hi, @tim-evans, a pre-release has been published:
|
27790b8
to
8c1f2e4
Compare
@copilot-robot prerelease |
Hi, @tim-evans, a pre-release has been published:
|
@copilot-robot prerelease |
Hi, @tim-evans, a pre-release has been published:
|
c867274
to
be28e1b
Compare
…to the layout attribute alignment was unclear as to what it was referring to and textAlignment works better in different contexts. layout is a new property that is in place of size and inset to be more extensible for designers, and to be more explicit that mixing size with block alignment (the inset property) doesn't actually work. this allows us to create a more consistent property that is available on more blocks without having this limited approach where size is on one thing, inset is on another, and some have both. having a single attribute for describing this will allow downstream consumers to handle this more easily.
this was never implemented
be28e1b
to
91fbe8c
Compare
The current attributes for our standard annotations have some messiness about them. This is an effort to clean up attributes and to provide a more standardized API that is consistent across more annotations. These decisions are made in conjunction with thinking about how our content is presented by our designers for sites like vogue.com, where some features that editors and writers use are not representable currently in our annotations.
We would like to provide more support and therefore, I'm introducing two major changes:
size
andinset
becomelayout
.These fields did not mix and match and allowing both properties made it confusing for developers as to which field took precedence. This also allows designers to add new types of layouts without having to change the data model.
alignment
becomestextAlignment
This is pretty self-explanatory, and is about clarity.
Note that
layout
as a property is added to many more blocks thansize
andinset
were on previously. This is intentional, since there are instances where editorial teams have used these features on these blocks. We may not provide them controls to change these properties, but we would like to fully support these and respect their decisions when it comes time to represent the data for our sites to use for rendering.