Homepage UI #3711

pavish · 2024-07-29T13:59:06Z

Fixes #3687

This PR implements home page changes based on the new permissions designs

Note:

The "collaborator" count on the card is not displayed. The store for it will be implemented in upcoming PRs and the count will be added on the UI once that is done.
The colors and layout are based on the existing UI and not the figma designs. This was decided in the design meetings.
Existing UI for the welcome page and empty states are unchanged. (The designs & colors are slightly different on the Figma file).

Reference: Figma file

Screenshots:

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the develop branch of the repository
My commit messages follow best practices.
My code follows the established code style of the repository.
I added tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no
visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

seancolsen

Mostly looks great!

I added some inline comments. Also:

I didn't like the global CSS you had in ConnectDatabaseModal so I pushed aa42c52 to refactor it out.

I've said this before, but I'd prefer that we avoid global CSS whenever it's easy to do so. And this is a case where I think it's pretty trivial to avoid.

The changes I pushed did alter the spacing of some things in minor ways, but I can see that the spacing in your version didn't match Figma pixel-for-pixel so I think we're okay. In fact, I'd say that the Figma spacing is actually a bit wonky.

In contrast, the global CSS in InstallationSchemaSelector is fine with me. Ideally we wouldn't have the global CSS there either, but that's harder to change, so I see it as an okay compromise to make for the sake of speed.

I've not been super happy with the abstractions that Svelte gives us to handle styling like in InstallationSchemaSelector. I'm curious if it will improve in Svelte 5. In particular I'm wondering if snippets will help. For example it would be neat if CheckboxGroup could accept an optional snippet for its ul.options element. Then we could pass in a snippet containing an entire ul element (with a CSS class). If this actually works, then I think it could become an incredibly powerful way to build low-level components which can be easily styled. I'm looking forward to playing with this at some point.
I also made a few other minor changes in aa42c52:
- I changed the left-most grid column to be auto instead of hard-coding it as 100px. (That way it's more adaptive to other code changes, and not reliant on px units, which I prefer to avoid.)
- I removed superfluous <div class="icon-holder"> element and class="content-holder" attribute, simplifying the markup.

I'd like to give it one more quick look before we merge.

seancolsen · 2024-07-31T19:30:03Z

mathesar_ui/src/api/rpc/databases.ts

  id: number;
  name: string;
  server_id: Server['id'];
 }

+/**
+ * TODO: Figure out a better place to move classes like these.


I like this idea, but I have some different opinions regarding the rules. It would be nice to chat about this on a call.

Also, it's nice seeing this sort of proposal written down, but process-wise I don't like using code comments as a discussion channel. I'd prefer to use a wiki PR or github issue.

We could resolve this in the proposal PRs and on call. I've removed the discussion code comments.

seancolsen · 2024-07-31T19:30:36Z

mathesar_ui/src/api/rpc/databases.ts

+ * }
+ * ```
+ */
+export class Database implements DatabaseResponse {


I'd like to put this code somewhere else, and I support putting it in @mathesar/models/database. That move doesn't necessarily need to happen in this PR though.

I'd like to make any change related to this after we figure out the store structures from the proposal PRs.

seancolsen · 2024-07-31T19:31:24Z

mathesar_ui/src/api/rpc/databases.ts

+  readonly server_id: number;
+
+  readonly server_host: string;
+
+  readonly server_port: number;


Can we use a single server: Server property instead of three separate server_* properties?

I'd like to make any change related to this after we figure out the store structures from the proposal PRs.

seancolsen · 2024-07-31T19:34:00Z

mathesar_ui/src/api/rpc/databases.ts

+
+  readonly server_port: number;
+
+  constructor(databaseResponse: DatabaseResponse, server: Server) {


I'm curious why you chose a class for Database (instead of an interface). I'd be inclined to use an interface since they're simpler and we don't have any methods. Maybe you foresee adding methods in the future? With an interface we'd lose the server id validation in the constructor, but that doesn't seem important to me. I don't have a strong opinion here though. Mostly just curious.

We could resolve this in the proposal PRs and on call.

seancolsen · 2024-07-31T19:35:24Z

mathesar_ui/src/systems/databases/create-database/ConnectExistingDatabase.svelte

+  const role = requiredField('');
+  const password = requiredField('');
+  const installationSchemas = requiredField<InstallationSchema[]>(['internal']);
+  const form = makeForm({ databaseName, installationSchemas });


I think makeForm should include all the form fields. That way the "Create Database" button doesn't get enabled until all the fields are filled out. Was there a reason you left some of the fields out of the form?

Good catch. I missed them while adding the fields. e6a70d9 fixes this.

…esar into homepage_ui

pavish added 11 commits July 29, 2024 19:26

Implment component to create new database

72e0258

Implement component to connect existing database

0a16b44

Implement connect database modal

716e981

Fix linting errors

b8aed1d

Update text and styling in connection creation modal

ddec88e

Implement common card component

f3716d6

Use cards in home page

655fa2c

Update text to Databases from Connections in home page

657e284

Styling improvements for header and cards

af451f5

Allow opening connect database modal in empty state

ef8b431

Show server information in database card

2ea8ca7

pavish force-pushed the homepage_ui branch from 9ec135e to 2ea8ca7 Compare July 30, 2024 21:55

pavish added 2 commits July 31, 2024 04:50

Add custom icons for creating & connecting databases

98e7f00

Remove autocomplete on password field

0c68c81

pavish assigned seancolsen Jul 30, 2024

pavish requested a review from seancolsen July 30, 2024 23:36

pavish marked this pull request as ready for review July 30, 2024 23:37

pavish added the pr-status: review A PR awaiting review label Jul 30, 2024

pavish added this to the Beta milestone Jul 30, 2024

pavish and others added 2 commits July 31, 2024 13:58

Change Database into a model class instead of a type

6ea3d8b

Refactor global CSS out

aa42c52

seancolsen reviewed Jul 31, 2024

View reviewed changes

seancolsen assigned pavish and unassigned seancolsen Jul 31, 2024

seancolsen added pr-status: revision A PR awaiting follow-up work from its author after review and removed pr-status: review A PR awaiting review labels Jul 31, 2024

pavish added 4 commits August 1, 2024 19:19

Merge branch 'develop' into homepage_ui

eeb5547

Merge branch 'develop' of https://github.com/mathesar-foundation/math…

25ff21c

…esar into homepage_ui

Add all fields to makeForm

e6a70d9

Remove discussion comment

bba0a68

pavish added pr-status: review A PR awaiting review and removed pr-status: revision A PR awaiting follow-up work from its author after review labels Aug 6, 2024

pavish assigned seancolsen and unassigned pavish Aug 6, 2024

pavish requested a review from seancolsen August 6, 2024 14:51

seancolsen approved these changes Aug 6, 2024

View reviewed changes

seancolsen added this pull request to the merge queue Aug 6, 2024

Merged via the queue into develop with commit 021a587 Aug 6, 2024
36 checks passed

seancolsen deleted the homepage_ui branch August 6, 2024 15:17

kgodey modified the milestones: Beta, Pre-beta test build #1 Sep 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Homepage UI #3711

Homepage UI #3711

pavish commented Jul 29, 2024 •

edited

Loading

seancolsen left a comment •

edited

Loading

seancolsen Jul 31, 2024

pavish Aug 6, 2024

seancolsen Jul 31, 2024

pavish Aug 6, 2024

seancolsen Jul 31, 2024

pavish Aug 6, 2024

seancolsen Jul 31, 2024

pavish Aug 6, 2024

seancolsen Jul 31, 2024

pavish Aug 6, 2024


		readonly server_port: number;

		constructor(databaseResponse: DatabaseResponse, server: Server) {

Homepage UI #3711

Homepage UI #3711

Conversation

pavish commented Jul 29, 2024 • edited Loading

Checklist

Developer Certificate of Origin

seancolsen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavish commented Jul 29, 2024 •

edited

Loading

seancolsen left a comment •

edited

Loading