Story supports-landscape attribute. #19849
Conversation
| if (uiState !== UIType.MOBILE || | ||
| this.getSupportedOrientations_() | ||
| .includes(ScreenOrientations.LANDSCAPE)) { | ||
| if (uiState !== UIType.MOBILE || this.isLandscapeSupported_()) { | ||
| this.storeService_.dispatch(Action.TOGGLE_LANDSCAPE, false); |
There was a problem hiding this comment.
Can we add a comment / todo here please? It's still a bit confusing for me to read this.
| value: "" | ||
| } | ||
| attrs: { | ||
| name: "supports-portrait" |
There was a problem hiding this comment.
I think we should hold off on adding this to the validation. I think it's better to add it when (if) we add validation tests & logic on the same PR.
There was a problem hiding this comment.
+1 I think it will give us more flexibility down the line to add this to validation only when we need it
There was a problem hiding this comment.
Validator team prefers to make things more permissive over time and not more restrictive. So I agree with holding off on adding something that is unsupported if it only gets removed later.
There was a problem hiding this comment.
I'm not sure it's safe to make code changes and validation changes in the same PR, since they can roll out separately. We wouldn't want a situation where the validation changes go live before the code changes, and then the feature doesn't work as expected.
/cc @Gregable to make sure that I'm not just spewing nonsense
| value: "" | ||
| } | ||
| attrs: { | ||
| name: "supports-portrait" |
There was a problem hiding this comment.
+1 I think it will give us more flexibility down the line to add this to validation only when we need it
|
@newmuis It's not nonsense but we haven't paid much attention to it as an issue to be concerned with. Typically most code features are behind experimental to start, not sure if that's the case here. That means validation can go in but the feature is documented as experimental. Does that only work at the component level or can it work for parts of the component? |
gmajoulet
force-pushed
the
supports_landscape
branch
2 times, most recently
from
2901e95
to
1aa9859
Compare
|
I created #19872 for the validation rules. PTAL |
gmajoulet
force-pushed
the
supports_landscape
branch
2 times, most recently
from
f7e7d20
to
3f01849
Compare
gmajoulet
force-pushed
the
supports_landscape
branch
from
3f01849
to
5c66cb6
Compare
Story
supports-landscapeattribute opt in and validation.Even though the
supports-portraitattribute doesn't do anything since we don't want publishers to be able to disallow portrait for now, I added it for consistency.#19270