-
Notifications
You must be signed in to change notification settings - Fork 1
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
Visual story 2 #304
Visual story 2 #304
Conversation
position: relative; | ||
height: 50px; | ||
margin: auto; | ||
`; |
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.
<CoverPage />
is the 1st card of the visual story. It uses the heroImage as background. It's not shown if hero image isn't present
src/helpers/invert-color/index.ts
Outdated
function padZero(str) { | ||
const zeros = new Array(2).fill("0"); | ||
return `${zeros}${str}`.slice(-2); | ||
} |
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 needed to invert theme colors used by text elements.. Visual stories use a dark background, so it's kinda like dark mode
<Theme tokens={tokens}>{children}</Theme> | ||
</StoryProvider> | ||
</ConfigProvider> | ||
); |
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.
Usually we use <Layout />
as context provider, but we can't use it here because
- we're using different theme tokens
- Layout adds a div (which sets width of story to 600px).. visual stories don't need this
<div grid-area="lower-third"> | ||
<StyledTextWrapper> | ||
<StyledHeadline>{headline}</StyledHeadline> | ||
{authorNames && ( |
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.
What about Date Published and Section name ?
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.
Ahead's visual stories doesn't have them right? is this a requirement?
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.
Nope. Thought we are doing it as per our header card.
Haven't added tests yet.. working on them and documentation |
src/atoms/visual-story/web-story-page-components/web-story-page-components.tsx
Outdated
Show resolved
Hide resolved
src/atoms/visual-story/web-story-page-components/web-story-page-components.tsx
Outdated
Show resolved
Hide resolved
Corresponding PR in framework quintype/quintype-node-framework#181 |
} | ||
}; | ||
``` | ||
|
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.
Do specify mandatory/optional for each of the below, along with he default values for optional ones.
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.
yes, will do that
docs-src/tutorials/visual-stories.md
Outdated
featureConfig: { | ||
visualStories: { | ||
autoAdvanceAfter: "5s", | ||
bookendUrl: "/foo.json", |
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.
Adding "/amp/api/v1/bookend.json" could serve as better example
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.
ok
import { withStoryAndConfig } from "../../../context"; | ||
|
||
export const AmpStoryPageBase = ({ config, story, children, ...props }: AmpStoryPageTypes) => { | ||
const autoAdvanceAfter = get(config, ["opts", "featureConfig", "visualStories", "autoAdvanceAfter"], null); |
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.
Setting default to null could be a problem as the document specifies it must be A positive amount of time
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 an attribute is null, React won't add it
source
if (!imgAnimationFeatCfg) return null; | ||
|
||
return { | ||
...(imgAnimationFeatCfg.animateIn && { "animate-in": imgAnimationFeatCfg.animateIn }), |
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.
Consider adding zoom-in
as default value for when animateIn is not passed in config.
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.
ok, can do that
huge: "40px" | ||
} | ||
}; | ||
}; |
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 reverts commit a15cd8d.
#301