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

#1367 add columns to sites list view #223

Merged
merged 14 commits into from
Sep 13, 2022

Conversation

mspratt-biot
Copy link

No description provided.

@@ -5,7 +5,16 @@ import { Site } from '../site/model';
import { DeviceDataSource } from '../device/DeviceDataSource';
import { SiteDataSource } from '../site/SiteDataSource';

export default function dataSources(deviceDAO: DAO<Device>, siteDAO: DAO<Site>): () => DataSources<object> {
interface DS {
Copy link

@slevertbiot slevertbiot Sep 12, 2022

Choose a reason for hiding this comment

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

I'd prefer DataSources as an interface name. Although that will conflict with DataSources. :)

Choose a reason for hiding this comment

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

Maybe ApiDataSources

Copy link
Author

Choose a reason for hiding this comment

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

sure

Comment on lines +13 to +19
export interface SitesArgs {
searchOptions: SearchOptions;
}

export interface SiteArgs {
id: string;
}

Choose a reason for hiding this comment

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

We may want to add docs to highlight the differences between Site and Sites

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean?

Choose a reason for hiding this comment

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

Why is a singular site only accepting an id while the plural site interface accepting an object with more args?

Copy link
Author

Choose a reason for hiding this comment

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

I thought is what typescript is for, just go see where this interface is used. if we do this, then we should comment all the interfaces everywhere. also, take a look at the device resolver, no types were ever provided into these functions params

Copy link
Author

Choose a reason for hiding this comment

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

should i just remove the types from the site resolver?

Comment on lines 40 to 43
site.validation?.summary.correct_devices.length,
site.validation?.summary.missing_devices.length,
site.validation?.summary.error_devices.length,
site.validation?.summary.extra_devices.length,

Choose a reason for hiding this comment

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

site.validation?.summary can be extracted to a local var and reused.

Copy link
Author

Choose a reason for hiding this comment

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

sure

Comment on lines +8 to +9
"resolveJsonModule": true,
"esModuleInterop": true,

Choose a reason for hiding this comment

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

What do these do?

Copy link
Author

Choose a reason for hiding this comment

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

They allow json to be imported as an es module like all the ways we import instead of requiring.

@mspratt-biot mspratt-biot merged commit 8501603 into master Sep 13, 2022
@mspratt-biot mspratt-biot deleted the #1367-AddColumnsToSitesListView branch September 13, 2022 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants