Greater-London-Authority / ldn-viz-tools

https://greater-london-authority.github.io/ldn-viz-tools/
1 stars 0 forks source link

Simple sidebar #214

Closed PaulioRandall closed 4 months ago

PaulioRandall commented 5 months ago

What does this change?

Adds a sidebar and a few inner sidebar components.

How?

How is it tested?

Storybook + currently being tested in map apps.

How is it documented?

Storybook + default sidebar content.

Are light and dark themes considered?

No.

Is it complete?

jamesscottbrown commented 5 months ago

Preview is at https://dev.ldn-gis.co.uk/storybook-simple-sidebar/?path=/docs/ui-sidebar--documentation

jamesscottbrown commented 5 months ago

This seems to work, but putting the map inside a SidebarContainer seems semantically wrong - is SidebarContainer meant to be a container for the whole page, rather than just the sidebar?

    <SidebarContainer>
        <div slot="content" class="h-full">
            <Map
                options={{
                    style: os_light_vts,
                    transformRequest: appendOSKeyToUrl(OS_KEY)
                }}
            />
        </div>

        <Sidebar slot="sidebar">
            <SidebarHeader title="Sidebar Overview" infoTitle="More info">
                <p>Introductory paragraph or two.</p>
                <p slot="info">More info content.</p>
            </SidebarHeader>

            <SidebarSection title="Section" infoTitle="More section info">
                <p>Section content.</p>
                <p slot="info">More section info content.</p>
            </SidebarSection>
        </Sidebar>
    </SidebarContainer>

image

Edit: I think what is clled a SidebarContainer here is what @ChrisKnightLDN calls an AppShell

jamesscottbrown commented 5 months ago

There doesn't seem to be a changest file for this PR, despite the " Have you included changeset file?" checkbox being checked.

The missing components were included in https://github.com/Greater-London-Authority/ldn-viz-tools/pull/92/files

ChrisKnightLDN commented 5 months ago

As it is now I feel it should follow the pattern in #92 which is consistent with figma. The construction of the sections in that story file seem logical. (Header may be a better name, though we really want to be consistent and as it is both figma and sidebar elements have been implemented using title - which I believe we debated at the time. We could re-open that debate, but as I said there needs to be consistency)

jamesscottbrown commented 5 months ago

I've made many changes in a new branch (which is still a WIP) and opened a draft PR: https://github.com/Greater-London-Authority/ldn-viz-tools/pull/216

ChrisKnightLDN commented 4 months ago

closing in favor of #216