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

fix(app): Fix robot list scroll clipping #2288

Merged
merged 2 commits into from
Sep 18, 2018
Merged

Conversation

Kadee80
Copy link
Contributor

@Kadee80 Kadee80 commented Sep 17, 2018

overview

closes #2046

Turns out the calc function for the height of the scrollable area in the sidebar was about 2rem too high, causing the scrollable area to flow off the page.

changelog

  • fix(app): Fix robot list scroll clipping

review requests

The sidebar is in the component library, so this change may (but shouldn't) affect the protocol designer. I did a quick check locally but please confirm.

  • app sidebars with scrollable content (robot list overflow in particular) render as expected
  • PD sidebars with scrollable content render as expected

@Kadee80 Kadee80 added bug app Affects the `app` project ready for review components Affects the `components` project labels Sep 17, 2018
@Kadee80 Kadee80 requested review from mcous and IanLondon September 17, 2018 15:47
Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

Dunno if it should be part of this PR or not, but the robot list looks pretty messy:

  • Unnecessary <div> between <SidePanel> and <RobotList>
  • <ol>s as direct (and therefore illegal) children of an <ol>
  • Nested <ol>s without list-style: none confusing the renderer

@@ -33,7 +33,7 @@

.panel_contents {
width: var(--sidebar-width);
height: calc(100vh - 2.625rem);
height: calc(100vh - 4.25rem);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the sidebar title is 3.5rem (56px). Is that not where this number was supposed to come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but there is a little fudging due to margins/spacing. I chose 4.25 rem because it worked with the robot item heights and spacing. 3.5rem cut it a little too close. Happy to revisit.

As for the robot list being messy - this got slugged in a while back, when we were thinking it was only an initial design solution - and we shoehorned in the existing comp lib list item.
@mcous I didn't consider refactoring this as part of this PR, but I say lets get that in there as well. How do we feel about ditching the comp lib list item implementation for the robot lists and considering a custom component for robot items?

Copy link
Contributor

@IanLondon IanLondon left a comment

Choose a reason for hiding this comment

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

🎈 🍚 🎈

@Kadee80 Kadee80 merged commit 28556ef into edge Sep 18, 2018
@Kadee80 Kadee80 deleted the app_fix-robot-list-scroll branch September 18, 2018 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project bug components Affects the `components` project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App: Robot list can't scroll all the way down
3 participants