-
Notifications
You must be signed in to change notification settings - Fork 31
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
What’s new page #55
What’s new page #55
Conversation
This pull request is automatically deployed with Now. |
Co-authored-by: shawnbot <shawnbot@github.com>
I'm ready for review on this, @emilybrick can you review the design? https://primer-style-ttaprfxfpb.now.sh/news |
Hey! Dropping some design feedback in here, happy to jump on a call though. Header
Filters
I'm going to take a minute to play with the list and label styling, and I'll follow up in a bit. Thanks for putting this together so quickly 🙌 ! |
@emilybrick I think this looks great for a first pass, I like the first illo 🙌. |
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.
Just left a few comments!
pages/news/index.js
Outdated
<Box p={4}> | ||
<Text fontSize={5}>What’s new</Text> | ||
<Box className="container-xl" p={3} py={7} style={{overflow: 'hidden'}}> | ||
<FlexContainer justifyContent="space-between" flexDirection={['column', 'column', 'row', 'row']}> |
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.
The team pages are built to have 5 breakpoints, we might want to match that here
pages/news/index.js
Outdated
<Text fontSize={5}>What’s new</Text> | ||
<Box className="container-xl" p={3} py={7} style={{overflow: 'hidden'}}> | ||
<FlexContainer justifyContent="space-between" flexDirection={['column', 'column', 'row', 'row']}> | ||
<Box maxWidth="550px"> |
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.
Could we use percentages here? I think the team page is using 1/2
src/Article.js
Outdated
<Text fontFamily="mono" f={1} color="blue.1"> | ||
<Link color="blue.1" href={url}> | ||
{articleDomain(url)} | ||
</Link>{' '} |
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 think this {' '}
will cause that jumping behavior we saw on the team page, use a
here instead
src/NewsList.js
Outdated
import styled from 'react-emotion' | ||
|
||
const FilterButton = styled(props => { | ||
const Tag = props.selected ? ButtonFill : ButtonOutline |
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 Tag isn't used, did you mean to set the tag prop in line 11 with it?
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 had this, and meant to delete the line, 👍
I started playing around with ways to make these sub pages more cohesive with the main index page... A few things I tried:
Let me know if this feels any better, I can always roll these changes back. Still working on mobile adjustments. |
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 think this looks great! 👏
Pushed up most changes we talked about yesterday aside from the mobile image sizes, which I think I need @jonrohan's help with 😬. I tried out using the team heading styles on the news page, but I'm not crazy about it: Specifically on mobile it feels out of place..I reverted back to Mono. I don't feel strongly that the relationship needs to be 1:1 with title styling on the page, but we can continue discussing it if others feel strongly. Done: To do:
|
Would love some feedback on this. If it's feeling good I'd love to merge today, since this branch has been hanging around for a bit. Based on feedback from Wednesday afternoon, I've been going back and forth with the typography on the /news list titles. Using the same size as our titles on the /team page felt really loud*. At the same time, the mono font doesn't feel as right here, as it seems like we are typically using mono for metadata or short strings (aside from the questions on the page). As a middle ground, I'm using the sans font for the titles, but one size down from the team page titles. Link: https://primer-style-fiipaarela.now.sh/ 👋 Updates as per convo w/ @broccolini:
|
done | ||
done | ||
|
||
echo "[ $(echo $output | sed -e "s/,$//g") ]" > ./src/data/releases.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.
It would be nice to pretty-print the JSON so that diffs were more useful, but that might be tricky given the way this is generated. 😬
Co-Authored-By: jonrohan <rohan@github.com>
* creating a whats new page * Adding some links * Updates to small text, adding link * Adding stateful news list with filterbuttons Co-authored-by: shawnbot <shawnbot@github.com> * Updating the ui and extracting * Lint * Adding border to fill button so the filter buttons don't jump * Change border color on hover * Making updates to what's new design and adding more data * Pluralizing the filters * Updates based on feedback * New line for prettier * Adding padding to nav * Using Flex instead of FlexContainer and FlexItem * Another FlexContainer to replace * Completing get releases data script and updating releases data * Blue.2 * fontSize prop * updated the design to the news page and removed unused assets * ran the linter, added target blank and fixed a typo in an article description * replaced 6px margin for 4px & 8px * updated descriptions * styling tweaks to svg illo and placement * reduced padding top on container xl down to 8 * fixed mobile margin and margin right for team image * linter fixes * styling changes for news page and team page image and content * reverted image sizing changes and renamed team svg * Formating and using the styled octicon * fixed sizing of svg and changed news list title font * styling changes to filter titles and list margin * updated get release data * new function to create titles for releases * changed truncation to start after 30 words * changed diamond color to blue 2 * Update src/whats-new.js Co-Authored-By: jonrohan <rohan@github.com> * linter fixes
This is a work in progress page to get news links into the app.
My approach to this is to get a data structure with links and content types listed out. They will be sorted by newest first. Displayed with a title, where the link lives, and date. For extra credit, I'm going to see if I can filter these links with some UI.