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(note): change parent note #148

Closed
wants to merge 19 commits into from
Closed

Feat(note): change parent note #148

wants to merge 19 commits into from

Conversation

kloV148
Copy link
Contributor

@kloV148 kloV148 commented Mar 23, 2024

What's new:

  • Added logic to update parent
  • Added button and textEdit to NoteSetting page

@kloV148 kloV148 changed the title Feat(note): edit parent note Feat(note): change parent note Mar 23, 2024
src/presentation/pages/NoteSettings.vue Outdated Show resolved Hide resolved
src/presentation/pages/NoteSettings.vue Outdated Show resolved Hide resolved
src/application/i18n/messages/en.json Outdated Show resolved Hide resolved
src/domain/noteSettings.service.ts Outdated Show resolved Hide resolved
src/presentation/pages/NoteSettings.vue Outdated Show resolved Hide resolved
*
* @param id - note id, if undefined returns an empty string
*/
function getNoteURL(id: NoteId | undefined): string {
Copy link
Member

Choose a reason for hiding this comment

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

I think its better to place in on a presentation level, in NoteSettings.vue

/**
* Link to the parent note
*/
parentURL: Ref<string>;
Copy link
Member

Choose a reason for hiding this comment

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

lets expose parentNote instead

src/domain/noteSettings.service.ts Show resolved Hide resolved
@TatianaFomina TatianaFomina linked an issue Mar 28, 2024 that may be closed by this pull request
2 tasks
function getParentURL(id: NoteId | undefined): string {
console.log('TEST' + parentNote.value);
if (parentNote.value === undefined) {
return 'null';
Copy link
Member

Choose a reason for hiding this comment

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

seems strange? what's the reason of "null" string?

* @param id - id of the note
*/
function getParentURL(id: NoteId | undefined): string {
console.log('TEST' + parentNote.value);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
console.log('TEST' + parentNote.value);

loadSettings(props.id);
const parentURL = ref<string>('');

/**
Copy link
Member

Choose a reason for hiding this comment

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

docs missed

Comment on lines 122 to 129
try {
await updateParent(props.id, parentURL.value);
} catch (error) {
if (error instanceof Error) {
window.alert(error.message);
}
throw error;
}
Copy link
Member

Choose a reason for hiding this comment

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

lets move try-catch to the application service


const invitationLink = computed(
() => `${import.meta.env.VITE_PRODUCTION_HOSTNAME}/join/${noteSettings.value?.invitationHash}`
);

loadSettings(props.id);
const parentURL = ref<string>('');
Copy link
Member

Choose a reason for hiding this comment

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

docs missed

// Extracts the ID from the URL. The ID is matches[1] as matches[0] is the full match
const parentId = matches[1];

const noteIdPattern = /^[a-zA-Z0-9-_]{10}$/;
Copy link
Member

Choose a reason for hiding this comment

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

why don't to put this regex to the first regex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because in the case where there is no slash at the end, we will take the first 10 characters of what comes after “/note/”. This may cause unexpected behavior
For example, when parentURL = '/note/1234567890abc' match will be like this match = '1234567890'. This could be the id of a real note that we don't want to be the parent of

*
* @param id - Note id
*/
const load = async (id: NoteId): Promise<void> => {
noteSettings.value = await noteSettingsService.getNoteSettingsById(id);
parentNote.value = (await noteService.getNoteById(id)).parentNote;
Copy link
Member

Choose a reason for hiding this comment

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

lets create a note ref that will store current note

@@ -21,6 +21,9 @@
"title": "Note Settings",
"customHostname": "Custom Hostname",
"hostnamePlaceholder": "example: landing.codex.so",
"parentNote": "Parent Note",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"parentNote": "Parent Note",
"parentNote": "Parent note",

@@ -17,6 +17,17 @@
type="primary"
@click="regenerateHash"
/>
<form-field
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<form-field
<FormField

@@ -10,6 +10,7 @@
"sourceType": "module",
"parser": "@typescript-eslint/parser"
},
"ignorePatterns": ["codex-ui/dist"],
Copy link
Contributor

Choose a reason for hiding this comment

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

your branch should not contain this change 🤔
seems like you did smth wrong while merging branches

@TatianaFomina
Copy link
Contributor

You have conflicts ☠️
image

@neSpecc neSpecc marked this pull request as draft April 25, 2024 17:01
@kloV148 kloV148 closed this Jun 29, 2024
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.

Change note parent
4 participants