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

refactor: add-fragments-content #983

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Rany0101
Copy link
Contributor

Summary

@Rany0101 Rany0101 requested a review from charIeszhao January 23, 2025 07:36
Copy link

Deploying logto-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: d30133e
Status: ✅  Deploy successful!
Preview URL: https://944452c9.logto-docs.pages.dev
Branch Preview URL: https://rany-add-fragment-content.logto-docs.pages.dev

View logs

Comment on lines +145 to +147
```json
Authorization: Basic ${Buffer.from(${id}:${secret}, 'utf8').toString('base64')}
```
Copy link
Member

@charIeszhao charIeszhao Jan 23, 2025

Choose a reason for hiding this comment

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

This is inaccurate I think.

  1. First of all, not all "traditional web" are JS based framework, the code example showed here is Node.js
  2. Even in Node.js, the code sample is incorrect. Should be
Authorization: `Basic ${Buffer.from(`${id}:${secret}`, 'utf8').toString('base64')}`

Comment on lines +1 to +2
/** eslint-disable no-irregular-whitespace \*/
/** eslint-disable no-irregular-whitespace \*/
Copy link
Member

Choose a reason for hiding this comment

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

These are not expected.

Comment on lines +236 to +244
```tsx
ssoIdentities: {
issuer: string;
identityId: string;
detail: {
[x: string]: Record<string, unknown>;
};
}[];
```
Copy link
Member

Choose a reason for hiding this comment

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

The code indent needs to be adjusted.

Comment on lines +314 to +335
```tsx
roles: {
id: string;
name: string;
description: string;
scopes: {
id: string;
name: string;
description: string | null;
resourceId: string;
resource: {
tenantId: string;
id: string;
name: string;
indicator: string;
isDefault: boolean;
accessTokenTtl: number;
}
}
[];
}
[];
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, would prefer using Array<{ key: type; key2: type }> when defining the types for a complex object, instead of []

Comment on lines +342 to +348
```tsx
organizations: {
id: string;
name: string;
description: string | null;
}[
```
Copy link
Member

Choose a reason for hiding this comment

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

Same problems as above

Comment on lines +354 to +361
```tsx
organizationRoles: {
organizationId: string;
roleId: string;
roleName: string;
}
[];
```
Copy link
Member

Choose a reason for hiding this comment

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

Are these types copied from API docs?

Copy link
Member

Choose a reason for hiding this comment

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

Should we use tables instead of json?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants