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: update to fundamental-styles@0.4.0-rc.3 #848

Merged
merged 11 commits into from
Jan 17, 2020

Conversation

meganaconley
Copy link
Contributor

Description

[BREAKING CHANGE]

Updates fundamental-styles to 0.4.0-rc.3, bringing in the following markup changes:

  1. SideNav refactor
  • Now uses nested-list styles and structure
  • Added utility nav
  1. Button changes
  • medium button removed
  1. Calendar changes
  • Month and year selectors changed from ul to table
  • Adds role="button" to each selectable calendar item
  • Adds aria-selected attribute to the currently-selected calendar item.

@meganaconley meganaconley requested a review from a team January 14, 2020 22:48
@netlify
Copy link

netlify bot commented Jan 14, 2020

Deploy preview for fundamental-react ready!

Built with commit 247ee9d

https://deploy-preview-848--fundamental-react.netlify.com

Copy link
Contributor

@jacobdevera jacobdevera left a comment

Choose a reason for hiding this comment

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

Everything is looking mostly good 👍

I'm not sure if it's just me, but if I resize the window in the docs site so that the SideNav on the left collapses, I can't scroll it up or down anymore after opening it again.

<Button disabled type='medium'>
Disabled
</Button>
</div>
<div className='frDocs-tile__break' />
Copy link
Contributor

Choose a reason for hiding this comment

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

An extra <div className='frDocs-tile__break' /> here

}
}

SideNavList.propTypes = {
children: PropTypes.node,
className: PropTypes.string,
compact: PropTypes.bool,
condensed: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

Will need prop descriptions for condensed, isUtility, and level

@greg-a-smith
Copy link
Contributor

@jacobdevera No, it's not just you. When the side nav collapses and it shown via the hamburger menu, it no longer scrolls.

Copy link
Contributor

@greg-a-smith greg-a-smith left a comment

Choose a reason for hiding this comment

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

I am seeing this weirdness on the Button page regardless of how wide I make the browser window.
Screen Shot 2020-01-16 at 9 15 08 AM

@@ -350,7 +391,7 @@ class Calendar extends Component {
for (let index = 0; index < 7; index++) {
weekDays.push(
<th className='fd-calendar__column-header' key={index}>
<span className='fd-calendar__day-of-week'>
<span className='fd-calendar__day-of-week' role='button'>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do the days of the week have a role of button? They aren't actually clickable, are they?

@@ -55,9 +57,10 @@ class SideNav extends Component {
SideNav.propTypes = {
children: PropTypes.node,
className: PropTypes.string,
compact: PropTypes.bool,
condensed: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs a prop description.

@skvale
Copy link
Contributor

skvale commented Jan 16, 2020

Related to the side-nav not scrolling from previous comments, the scrollbar changed elements

master branch

Screen Shot 2020-01-16 at 10 10 47 AM

this branch

Screen Shot 2020-01-16 at 10 09 29 AM

@greg-a-smith
Copy link
Contributor

@skvale That's actually part of this PR's change. The Side Navigation component (which is what the site's left nav is built with) has been refactored and is now using different classes.

@meganaconley
Copy link
Contributor Author

New commit fixes:

  • unnecessary button role on Calendar
  • Button page layout
  • SideNav scroll issue
  • Prop descriptions

@greg-a-smith
Copy link
Contributor

@meganaconley The PR feedback changes looked good except for the side nav scrolling. It did scroll this time, however, now even the search box scrolled with it -- that is supposed to stay fixed. Instead of more comments and screenshots, I contributed a few changes. I set the sidebar width to 15rem to match the side nav styles (to get rid of the gray gutter on the right of the side nav). I moved the overflow definitions so only the side nav items would scroll.

Copy link
Contributor

@greg-a-smith greg-a-smith left a comment

Choose a reason for hiding this comment

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

Assuming the build passes, these changes look good. I am part author now so I'll give it a partial ⛵️. If my contributions look good, feel free to approve and merge.

@meganaconley
Copy link
Contributor Author

Thanks for the help @greg-a-smith!

Copy link
Contributor

@bcullman bcullman left a comment

Choose a reason for hiding this comment

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

🚢

@meganaconley meganaconley merged commit 6a63a21 into master Jan 17, 2020
@meganaconley meganaconley deleted the feat/update-fs-0.3.1 branch January 17, 2020 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants