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.

Sign up for GitHub

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

Jump to bottom

Refactor/update settings page #124

Merged
merged 25 commits into from
Jun 4, 2021
Merged

Refactor/update settings page #124

merged 25 commits into from
Jun 4, 2021

Conversation

keitarofreeman
Copy link
Contributor

Related issues:
#121
#120
#104

@todo
Copy link

todo bot commented May 25, 2021

use currentUserId to retrive tenants only for current user

backoffice/src/context/TenantsContext.tsx

Lines 40 to 45 in 514ca02

const { data, loading, error } = useGetTenantsQuery(); // TODO: use currentUserId to retrive tenants only for current user
const tenants =
!loading && !error && authContext.isLoggedIn() && data
? data.tenants.items
: [];
// TODO: handle errors

This comment was generated by todo based on a TODO comment in 514ca02 in #124. cc @tabetalt.

@todo
Copy link

todo bot commented May 25, 2021

handle errors

backoffice/src/context/TenantsContext.tsx

Lines 45 to 50 in 514ca02

// TODO: handle errors
if (error) console.log(error);
const currentTenant = tenants.length > 0 ? tenants[0] : null;
function updateRequired() {
updateTenant(!tenantUpdateState);

This comment was generated by todo based on a TODO comment in 514ca02 in #124. cc @tabetalt.

@lgtm-com
Copy link

lgtm-com bot commented May 25, 2021

This pull request introduces 6 alerts when merging 6128ce3 into d161548 - view on LGTM.com

new alerts:

  • 6 for Unused variable, import, function or class

simenandre
simenandre reviewed May 25, 2021
View reviewed changes
Copy link
Member

@simenandre simenandre left a comment

Choose a reason for hiding this comment

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

Consider using less type-casting. I think we should try to remove the amount of as XXXX to use a function, this way we have one place to do type-casting.

.eslintrc.json Outdated
},
"parser": "@typescript-eslint/parser"
}
{
Copy link
Member

Choose a reason for hiding this comment

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

What is happening in this? 🤷‍♂️

codegen.yml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/App.tsx Outdated
@@ -6,6 +6,7 @@ import { theme, Skeleton } from '@tabetalt/kit';
import { getRoutes } from './routing';
import gqlClient from './api/client';
import { useAuth } from './context/AuthContext';
import { TenantsProvider } from './context/TenantsContext';
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to use singular naming tenants → tenant

src/context/TenantsContext.tsx Outdated
Comment on lines 16 to 18
updateRequired: () => {
// Dummy function
},
Copy link
Member

Choose a reason for hiding this comment

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

What the intention with this?

src/context/TenantsContext.tsx Outdated Show resolved Hide resolved
src/helpers/index.ts Outdated Show resolved Hide resolved
src/helpers/index.ts Outdated
vatRateObject: DineroObject,
price: string
): string => {
const dineroPrice = Dinero(moneyFromString(price) as DineroObject);
Copy link
Member

Choose a reason for hiding this comment

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

This looks a bit wordy. Maybe we should make a function like mentioned above? dineroFromMoney or something? 🤷‍♂️

src/modules/catalog/product/ProductCreate.tsx
@@ -37,7 +37,6 @@ const ProductCreate: React.FC = () => {
};

if (values.categories) {
console.log(values.categories);
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch!

src/modules/catalog/products/Products.tsx Outdated Show resolved Hide resolved
@simenandre
Copy link
Member

This pull request introduces 6 alerts when merging 6128ce3 into d161548 - view on LGTM.com

new alerts:

  • 6 for Unused variable, import, function or class

Should probably fix these before merging too. 👍

@lgtm-com
Copy link

lgtm-com bot commented May 26, 2021

This pull request introduces 6 alerts when merging d42ecd8 into d161548 - view on LGTM.com

new alerts:

  • 6 for Unused variable, import, function or class

RomanTkachev and others added 2 commits May 26, 2021 21:00
refactor: Singular naming for tenant context.
af087c7
@keitarofreeman @simenandre
Update src/helpers/index.ts
5c3be80 
Co-authored-by: Simen A. W. Olsen <cobraz@cobraz.no>
@lgtm-com
Copy link

lgtm-com bot commented May 26, 2021

This pull request introduces 7 alerts when merging 5c3be80 into d161548 - view on LGTM.com

new alerts:

  • 6 for Unused variable, import, function or class
  • 1 for Useless conditional

@keitarofreeman @simenandre
Update src/modules/catalog/products/Products.tsx
25ff5c8 
Co-authored-by: Simen A. W. Olsen <cobraz@cobraz.no>
@lgtm-com
Copy link

lgtm-com bot commented May 26, 2021

This pull request introduces 7 alerts when merging 25ff5c8 into d161548 - view on LGTM.com

new alerts:

  • 6 for Unused variable, import, function or class
  • 1 for Useless conditional

@lgtm-com
Copy link

lgtm-com bot commented May 27, 2021

This pull request introduces 3 alerts when merging 967d443 into d161548 - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented May 27, 2021

This pull request introduces 3 alerts when merging 4ca0875 into d161548 - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented May 27, 2021

This pull request introduces 1 alert when merging 1424b2f into d161548 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

simenandre
simenandre approved these changes Jun 3, 2021
View reviewed changes
Copy link
Member

@simenandre simenandre left a comment

Choose a reason for hiding this comment

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

👍

@keitarofreeman keitarofreeman merged commit c4f519c into master Jun 4, 2021
This was referenced Jun 4, 2021
Open
Open
This was referenced Jun 7, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simenandre simenandre simenandre approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@keitarofreeman @simenandre