evidence-dev / evidence

Business intelligence as code: build fast, interactive data visualizations in pure SQL and markdown
https://evidence.dev
MIT License
3.44k stars 167 forks source link

Layout updates #1086

Closed mcrascal closed 7 months ago

mcrascal commented 9 months ago

see discussion and background on #1079

changeset-bot[bot] commented 9 months ago

šŸ¦‹ Changeset detected

Latest commit: 9b8346f02221e0e3f321577f332c3b6a4723f453

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages | Name | Type | | --------------------------------- | ----- | | @evidence-dev/component-utilities | Minor | | @evidence-dev/core-components | Minor | | @evidence-dev/evidence | Major | | @evidence-dev/preprocess | Minor | | @evidence-dev/tailwind | Minor | | evidence-docs | Minor | | @evidence-dev/components | Minor | | evidence-test-environment | Minor | | ui-tests | Minor |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel[bot] commented 9 months ago

The latest updates on your projects. Learn more about Vercel for Git ā†—ļøŽ

Name Status Preview Comments Updated (UTC)
evidence-docs āœ… Ready (Inspect) Visit Preview šŸ’¬ Add feedback Oct 12, 2023 1:35am
netlify[bot] commented 9 months ago

Deploy Preview for evidence-development-workspace ready!

Name Link
Latest commit 9b8346f02221e0e3f321577f332c3b6a4723f453
Latest deploy log https://app.netlify.com/sites/evidence-development-workspace/deploys/65274d1fe1c0ce0008241863
Deploy Preview https://deploy-preview-1086--evidence-development-workspace.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

hughess commented 9 months ago

This is looking awesome! Added some notes below. Iā€™ve made note of anything I saw that looked off or where I had questions, so thereā€™s some comments in here that arenā€™t directly related to these changes. I donā€™t feel strongly about any of this - just surfacing all my reactions:

csjh commented 8 months ago

Seems like removing isTemplated in +layout.svelte broke template page breadcrumbs

mcrascal commented 8 months ago

@csjh good shout.

mcrascal commented 8 months ago

Hi folks, this is ready for the final review. Thanks @hughess and @archiewood for the feedback so far. @ItsMeBrianD it would be really helpful to get your eyes on this as well.

Feedback wanted

For the final review, I am most interested in major breaking issues that need to be fixed before shipping this.

The changes in this PR are almost all css, so for any issues you discover in functionality, please just confirm that the issue isn't also present in the current release.

After this merge

I've tried to make minimal changes to existing components in this PR, just to keep things working. Where I did, this was usually a tailwind conversion inside existing classes.

While I think this looks dramatically better than what we currently have, there is more work to do to get all of the components to be really cohesive with the new layout. I want to tackle these types of changes in smaller chunks after we ship this .

Please hold off on opening that type of ticket for the next few days (e.g. modal doesn't fit with new layout). I'll do a sweep and organize tickets for these items so we can tackle them one at a time after we've shipped this.

Major changes since Sean's review

Typography

Components

Printing

Tests and deps

@ItsMeBrianD

  1. can you take a look and confirm I haven't left behind any deps issues. I think this PR now reflects the changes that also landed in next.
  2. Can you also rewire the UI tests to target the new elements?

ClosesĀ #712,Ā closesĀ #739,Ā closesĀ #777,Ā closesĀ #501

hughess commented 8 months ago
mcrascal commented 7 months ago

I think we're good to go -- @hughess, @archiewood take another look and lmk? šŸ¤ž

The page seems to need some colour

Base font size might be a bit big?

Current state CleanShot 2023-09-19 at 18 32 01@2x

This PR CleanShot 2023-09-19 at 18 32 41@2x

Alert - sticky alert isnā€™t working

Print PDF

I think I have this figured out. Screenshots of a few browsers / widths below. This PR also fixes #1212.

Current state

Chrome, Mac OS Sonoma

Ultrawide:

b8fc6628-4ff9-4781-a215-4bc6b0fe274b

400px:

fff41642-6890-4d41-851a-72eb9a7c26b8

Safari, Mac OS Sonoma

Ultrawide:

CleanShot 2023-09-29 at 10 51 20@2x

400px:

CleanShot 2023-09-29 at 10 53 26@2x

Chrome, Windows:

Ultrawide:

CleanShot 2023-09-29 at 10 53 57@2x

400px:

CleanShot 2023-09-29 at 10 54 24@2x

As of this PR

Print targets are the same regardless of the initial page width

Chrome, Mac

CleanShot 2023-09-29 at 10 55 23@2x

Safari, Mac

CleanShot 2023-09-29 at 10 55 52@2x

Chrome, Windows

CleanShot 2023-09-29 at 10 56 43@2x

Styling Issues

Code background seems too light - barely noticeable against the white background on my desktop monitor

I can't reproduce this, propose we revisit if other folks are running into this issue.

Deltas - strong preference for the rounded glyphs included with apple system UI

Current State

Chrome Windows

Pasted image 20230929161050

Chrome Mac

CleanShot 2023-09-29 at 16 11 21@2x

This PR - mac and windows

CleanShot 2023-09-29 at 15 32 55@2x

Big value

Compiled / written buttons and toggle

Details component

Links

Sidebar selected page not distinctive enough

Current state on the query chaining page

CleanShot 2023-09-17 at 19 12 41@2x 1

This PR on the query chaining page

CleanShot 2023-09-29 at 15 00 07@2x

Other

mcrascal commented 7 months ago

Thanks @archiewood !

  1. This is intentional. The headers on those chart testing pages are bare h1 tags, not actual markdown. This PR removes most of the global styles from our project, and isolates user markdown to the markdown class, so that when we're building components we're not fighting with a bunch of global styles we aimed at markdown, and when we're making changes to markdown styling, we're not inadvertently breaking components.
  2. This is present in main - there's a row link and a link column, if you click on the link column both navigation events fire. I'm not even sure if this is a bug, it's just a weird case.
  3. Young @ItsMeBrianD added these when he was testing reactivity on chart annotations. Present in main.
  4. I don't feel strongly about it. I kind of like them as small-ish footnotey things.
ItsMeBrianD commented 7 months ago

Young @ItsMeBrianD added these when he was testing reactivity on chart annotations. Present in main.

Was this @csjh?

mcrascal commented 7 months ago

Young @ItsMeBrianD added these when he was testing reactivity on chart annotations. Present in main.

Was this @csjh?

Ah maybe!

csjh commented 7 months ago

Yeah the animation is is my creation and intended, just a little test to make sure they don't duplicate or otherwise with reactivity

hughess commented 7 months ago

This is looking sweet. Here are my notes:

Potential Issues

  1. The evidence logo looks like it's not vertically centered - looks lower than the 3-dot menu

  2. Errors are now being printed to the console for horizontal bar charts (canā€™t use date axis), but only on server side navigation. This is what I see when I navigate to the chart-testing/dateSeries page:

    chart-error-printing
  3. Alerts: default alert appears to have different font size and spacing

    alert-sizing
  4. Delta columns in table donā€™t look vertically centered - they look aligned to the top of the other text

    delta-vertical
  5. Deltas - negative arrow isn't vertically centered, which creates an illusion for me where I see the arrow extending below the text it's beside

    delta-down
  6. Details component - I think the vertical spacing needs adjustment. The way it sits right now makes it look coupled to the content below it, whereas I think in most cases, you'd want it to be coupled with the content above (e.g., under a header, or under a chart)

    details-spacing
  7. Details content: agree with Archie that font of content looks a bit big relative to the title of the component. Also agree that it should be a smaller footnotey thing. If possible, Iā€™d probably go with a smaller base font size for content within a details component

Minor Things

  1. Print PDF causes the page to hang for a second, then you see the charts change width. On closing print dialog, you see them shift back print-shift

  2. SQL font within query viewers looks a bit large

    sql-font-size
  3. Lists - having equal spacing above and below the list makes it look "on it's own" vs. having it coupled with the text leading into the list. I think I'd expect the spacing on the top to be slightly less than the spacing on the bottom

    list-spacing

Fixed

This is what I was seeing before for the inline code style - new one looks perfect:

Before

code-style-old

After

code-style-new

Questions

mcrascal commented 7 months ago

Thanks @hughess!!

  1. Fixed
  2. This is present in main
  3. Fixed
  4. I have rolled the implementation back to just use font glyphs and hard coded system ui into them. Windows users will continue to get hard triangles. Juice not worth the squeeze on further iteration with custom svgs IMO.
  5. Glyphs also aligned baseline in main.

    1. Current

    2. CleanShot 2023-10-02 at 15 59 13@2x

    3. Custom SVG

    4. CleanShot 2023-10-02 at 15 59 59@2x

    5. System UI glyph

    6. CleanShot 2023-10-02 at 16 00 29@2x

  6. Reverted the spacing back to current
  7. Wont fix for now -- I am worried we're going to get into conflicting styles with components added as children, happy to revisit.
mcrascal commented 7 months ago

@hughess Re: question on bare html

yes, this will cause people who have inlined bare html to have unstyled components (base font and size still). If they want them styled as if they came from markdown they can add the markdown class. I'll add a note in the docs, and update the one instance where we do this in the template (lists in a loop).

hughess commented 7 months ago

Sounds great to me!