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

Improve styling of new sidebar guides #1406

Merged
merged 17 commits into from
Jun 11, 2021

Conversation

OskarDamkjaer
Copy link
Contributor

@OskarDamkjaer OskarDamkjaer commented May 7, 2021

Preview @ http://guide_polish.surge.sh

Problems addressed:

  • Fiddling with dots in lists created an extra line break
  • Missing bottom padding for long guides
  • Low e2e test coverage
  • Pagination for with numbers
  • Font ligatures on in guides make them harder to read for beginners.
  • Some header font is slow to load which looks odd (True for all of sidebar)
  • Prevent running cypher from guides on click

I'll annotate the source as to what solves what.

package.json Outdated
@@ -29,7 +29,6 @@
"e2e-local": "CYPRESS_E2E_TEST_ENV=\"local\" cypress run",
"e2e-local-open": "CYPRESS_E2E_TEST_ENV=\"local\" cypress open",
"format": "prettier-eslint --write 'src/**/!(*.min).{js,jsx,ts,tsx,css,json}' 'e2e_tests/**/*.{js,jsx,ts,tsx,css,json}' 'scripts/**/*.{js,jsx,ts,tsx,css,json}' 'build_scripts/**/*.{js,jsx,ts,tsx,css,json}'",
"jest": "jest --coverage",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this in package.json we can run yarn jest to run jest without flags.

@@ -3,9 +3,7 @@ import styled from 'styled-components'
export const NoBulletsUl = styled.div`
list-style-type: none;
`
export const BulletsInsideLi = styled.li`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unused

@@ -437,6 +437,9 @@ export const StyledSidebarSlide = styled.div.attrs({
list-style-position: inside;
padding-left: 0;
margin-top: 0.5em;
p {
display: inline-block;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before we added list-style-position: inside;

in a

  • was shown inline, added this to keep them inline.

  • type DotDotDot = '...'

    const range = (from: number, to: number) =>
    Array.from({ length: from ? to - from + 1 : to }, (_, i) => i + 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.

    Perhaps it's better to use the lodash range instead

    @@ -19,7 +19,7 @@ import React, { useState } from 'react'
    import styled from 'styled-components'

    const StyledHeaderText = styled.div`
    font-family: 'Open Sans';
    font-family: ${props => props.theme.drawerHeaderFontFamily};
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    Adds fallback fonts, so we don't get as drastic font flashes

    @OskarDamkjaer OskarDamkjaer requested a review from jharris4 May 7, 2021 10:31

    /* global Cypress, cy, before */

    describe('Play command', () => {
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    Misleading name of this test suite! ;-)

    <>
    {paginationHelper(itemCount, selectedIndex).map(
    (slideIndexOrDots, index) => {
    if (slideIndexOrDots === '...') {
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    Since you've already declared a type for this, can't you use that or a const? Feels like it'd make it easier to change the dots later if the literals weren't mixed throughout the code...

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    I've considered using null instead 🤔

    @OskarDamkjaer OskarDamkjaer requested a review from jharris4 May 10, 2021 09:00
    Copy link
    Contributor

    @jharris4 jharris4 left a comment

    Choose a reason for hiding this comment

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

    LGTM Love the TDD on the pagination component

    @OskarDamkjaer OskarDamkjaer merged commit 5055844 into neo4j:master Jun 11, 2021
    @OskarDamkjaer OskarDamkjaer changed the title Guide polish Improve styling of new sidebar guides Jun 22, 2021
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants