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

feat: add list view prototype #6

Merged
merged 9 commits into from
Oct 15, 2020

Conversation

iamstiil
Copy link
Collaborator

@iamstiil iamstiil commented Sep 18, 2020

This PR adds a list view to the main page.

Copy link
Owner

@TejasQ TejasQ left a comment

Choose a reason for hiding this comment

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

Please always add a description to the PR.

components/BookCard.tsx Outdated Show resolved Hide resolved
@iamstiil iamstiil requested a review from TejasQ September 20, 2020 10:56
Copy link
Owner

@TejasQ TejasQ left a comment

Choose a reason for hiding this comment

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

This is amazing! Thank you for your great work! I've added some pointers to help us have a bit more reusability.

@@ -0,0 +1,21 @@
import * as React from 'react';
Copy link
Owner

Choose a reason for hiding this comment

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

Normally, we don't need to do this. This should work just fine.

Suggested change
import * as React from 'react';
import React from 'react';

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had someone point this out to me once: facebook/react#18102

import * as React from 'react';

// eslint-disable-next-line @typescript-eslint/no-empty-interface
interface BookCardProps {}
Copy link
Owner

Choose a reason for hiding this comment

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

Let's remove this and add it once we need it. No premature code please.

Also, let's use types over interfaces.

Interfaces have stung me in the past in multiple ways. For example:

  • You can declare the same interface multiple times and TS will just go with it and create some weird intersection. All type names are expected to be unique like variables.
  • Interfaces are more common with OOP (class Thing implements MyInterface). I'd like this project to be more functional than object-oriented.
  • Interfaces and types are composable the same way, but types have unique constraints.
  • VS Code only: when hovering on a type, you get a FULL DESCRIPTION of its schema. When hovering on an interface, you just see interface MyInterface without any idea what it actually is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should add this to a wiki page, just in case :)

@@ -0,0 +1,77 @@
import * as React from 'react';
Copy link
Owner

Choose a reason for hiding this comment

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

See previous comment about the imports.

components/ViewToggle.tsx Outdated Show resolved Hide resolved
components/ViewToggle.tsx Outdated Show resolved Hide resolved
components/ViewToggle.tsx Outdated Show resolved Hide resolved
components/ViewToggle.tsx Outdated Show resolved Hide resolved
pages/_app.tsx Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@iamstiil iamstiil requested a review from TejasQ September 24, 2020 17:49
@TejasQ TejasQ mentioned this pull request Oct 15, 2020
@TejasQ TejasQ merged commit eb80a14 into TejasQ:master Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants