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

feat(theme): implement release notes page layout #336

Merged
merged 22 commits into from
Apr 8, 2020

Conversation

emmenko
Copy link
Member

@emmenko emmenko commented Apr 2, 2020

This PR implements the page layout structure for the release note pages. The layout is the same as for the content pages, except for the absence of the site navigation and the page filters (to be done separately).

This does not implement the designs of the release notes, only the layout.

@vercel
Copy link

vercel bot commented Apr 2, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/commercetools/commercetools-docs-kit/jqmw4sld4
✅ Preview: https://commercetools-docs-kit-git-nm-theme-release-notes-layout.commercetools.now.sh

@emmenko emmenko added the 🚀 Type: New Feature Something new label Apr 2, 2020
@vercel vercel bot temporarily deployed to Preview April 2, 2020 12:52 Inactive
@emmenko

This comment has been minimized.

@vercel vercel bot temporarily deployed to Preview April 2, 2020 13:35 Inactive
@vercel vercel bot temporarily deployed to Preview April 2, 2020 13:51 Inactive
@emmenko emmenko force-pushed the nm-theme-release-notes-layout branch from 5bd4ad4 to ccb1cbb Compare April 3, 2020 19:22
@vercel vercel bot temporarily deployed to Preview April 3, 2020 19:22 Inactive
@vercel vercel bot temporarily deployed to Preview April 3, 2020 19:26 Inactive
@@ -19,6 +19,7 @@ const defaultOptions = {
beta: false,
gaTrackingId: undefined,
excludeFromSearchIndex: true,
hasReleaseNotes: false,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link to the release notes is toggled by this config flag.

node,
name: 'isGlobalBeta',
value: Boolean(pluginOptions.beta),
});
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are used by all templates, because of the sidebar. Therefore we always generate those fields.

Comment on lines 174 to 181
fragment MdxPageFields on MdxFields {
slug
title
beta
isGlobalBeta
excludeFromSearchIndex
hasReleaseNotes
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These fields are shared by all pages. Extra fields (for example for the release notes) should be listed in the specific query.

Comment on lines 241 to 320
component: isOverviewPage
? require.resolve('./src/templates/release-notes-list.js')
: require.resolve('./src/templates/release-notes-detail.js'),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some back and forth I think it's better to just have two separate templates, it makes things easier to implement and separates a bit the concerns, as the pages are indeed different.

@@ -0,0 +1,5 @@
import React from 'react';

const ReleaseNotesSubscribeLinks = () => <div>{'TODO subscribe links'}</div>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we will add the "subscribe" links (rss feed, etc.). To be done in a separate PR.

@@ -0,0 +1,51 @@
import React from 'react';

const useLayoutState = () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved all this state into a hook, to make is easier to reuse across the different layout components.

Comment on lines +22 to +26
const LayoutHeaderLogo = () => (
<Container>
<LogoButton />
</Container>
);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just extracted for better reusability


const LayoutPageReleaseNotesFilters = () => (
<GridContainer>
<StickyContainer>{'TODO: filters'}</StickyContainer>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is where the filters will be implemented. To be done in a separate PR.

Comment on lines 63 to 86
const linkStyles = `
border-left: ${designSystem.dimensions.spacings.xs} solid ${designSystem.colors.light.surfaceSecondary1};
padding-left: calc(${designSystem.dimensions.spacings.m} - ${designSystem.dimensions.spacings.xs});
text-decoration: none;
color: ${designSystem.colors.light.textSecondary};
display: flex;
flex-direction: row;
align-items: flex-end;

:hover {
color: ${designSystem.colors.light.linkNavigation} !important;
}

> * + * {
margin: 0 0 0 ${designSystem.dimensions.spacings.s};
}
`;
const activeLinkStyles = `
border-left: ${designSystem.dimensions.spacings.xs} solid ${designSystem.colors.light.linkNavigation} !important;
color: ${designSystem.colors.light.linkNavigation} !important;
`;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted into variables for better readability

Comment on lines +88 to +92
customStyles,
customActiveStyles,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two props are new, to allow to override some of the link styles.

customActiveStyles: PropTypes.string,
};

const SidebarLinkWrapper = (props) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split the SidebarLink component into two, with the wrapper containing the logic for the scrolling position.

The base link component is use for the release notes link.

};

const Sidebar = (props) => {
const SidebarNavigationLinks = (props) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This component is also extracted and contains the normal navigation links.

{props.hasReleaseNotes && (
<ReleaseNotesTitle>
<SidebarLink
to="/releases"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the link to the release notes.

</SidebarLink>
</ReleaseNotesTitle>
)}
{props.hasReleaseNotes && isReleasePage ? (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this condition is true, we render the "back" link

Comment on lines +44 to +54
<SpacingsInline alignItems="center">
<AngleLeftIcon size="medium" color="primary" />
<Link href="/releases" noUnderline={true}>
<span
css={css`
font-size: ${designSystem.typography.fontSizes.small};
`}
>
{'Back to all releases'}
</span>
</Link>
</SpacingsInline>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The detail page has this "back" link at the top

@vercel vercel bot temporarily deployed to Preview April 3, 2020 19:46 Inactive
@vercel vercel bot temporarily deployed to Preview April 3, 2020 20:10 Inactive
Comment on lines +13 to +17
// NOTE: release notes content can only have headings starting from h4.
h1: Markdown.withAnchorLink(Markdown.H4),
h2: Markdown.withAnchorLink(Markdown.H5),
h3: Markdown.withAnchorLink(Markdown.H6),
h4: Markdown.withAnchorLink(Markdown.H6),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the mapped list of headings for the release notes content.

cc @nkuehn

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we ended up saying that release notes should not have any headings but I think it's a good idea to treat that decision as a technical writing / lectoring convention but have the implementation still render them as a lower "level". You never know what comes up in the future.

H4 is correct to start with since we skip H2 in that page layout.

internal: { mediaType: { eq: "text/mdx" } }
}
) {
allContentPage {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now have our own custom query fields.

Comment on lines +299 to +301
allReleaseNotePage {
totalCount
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use this to determine if there are any release notes in the website.

We can then be more specific and define some filters to which release notes we want to include. For example, we can exclude all the ones that are not marked as published.

// You can use the values in this context in
// our page layout component
component: isOverviewPage
? require.resolve('./src/templates/release-notes-list.js')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The release overview page needs to be rendered here now. It still has its own template but from a graphql type perspective is considered a "content page" and therefore is queried with all other content pages.

The file is now src/content/releases.mdx instead of src/releases/index.mdx, for the reasons mentioned above.

Comment on lines +57 to +61
contentPage(slug: { eq: $slug }) {
title
beta
isGlobalBeta
excludeFromSearchIndex
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those fields should be queried here, we don't need to pass them with the page context.
Also note the we are using the contentPage query field.

title
isGlobalBeta
excludeFromSearchIndex
date(formatString: "YYYY-MM-DD")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can format the date here.

Comment on lines +99 to +109
allReleaseNotePage(sort: { order: DESC, fields: date }) {
nodes {
slug
title
date(formatString: "YYYY-MM-DD")
description
type
topics
body
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we query all release notes and sort them by date descending.

@vercel vercel bot temporarily deployed to Preview April 7, 2020 20:36 Inactive
Copy link
Collaborator

@nkuehn nkuehn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! It's definitely the best way to directly switch to using a fully custom graphQL API surface. It matches the experiences we made in building the API types stuff. Plus I really really like the look of the left menu now that it's even cleaner.

!! One bug remains when opening a build (not development): The main nav link to the release notes has a flash of unstyled content (it moves from right to left).

To not block this another full day I leave this here as an approval, please manage yourself whether to directly fix it or to open a separate issue to track it.

Comment on lines +46 to +51
parent {
id
... on File {
relativePath
ext
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now necessary because the slug is not created as a MDX field anymore.

@vercel vercel bot temporarily deployed to Preview April 7, 2020 20:48 Inactive
@vercel vercel bot temporarily deployed to Preview April 8, 2020 07:36 Inactive
@emmenko
Copy link
Member Author

emmenko commented Apr 8, 2020

@nkuehn the FOUC bug is fixed now, I'll merge the PR once CI passes.

@emmenko emmenko merged commit d4a86bc into master Apr 8, 2020
@emmenko emmenko deleted the nm-theme-release-notes-layout branch April 8, 2020 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants