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

Adds scheme names/labels section #153

Merged
merged 3 commits into from
Jan 3, 2025
Merged

Adds scheme names/labels section #153

merged 3 commits into from
Jan 3, 2025

Conversation

aarongundel
Copy link
Collaborator

Adds scheme names/labels section

@aarongundel aarongundel linked an issue Dec 13, 2024 that may be closed by this pull request
@aarongundel aarongundel requested a review from chrabyrd December 16, 2024 21:21
@aarongundel
Copy link
Collaborator Author

This will need to merge into main before/at the same time as JC's form editor ticket.

@aarongundel aarongundel marked this pull request as ready for review December 18, 2024 17:11
Copy link
Contributor

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

works well, just some code design questions

Copy link
Contributor

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

There appear to be some warnings in the console relating to various Scheme* components. Mind giving those a look?

But yeah overall looking good 👍

/>
<Column
field="appellative_status_ascribed_name_content"
header="Label"
Copy link
Contributor

Choose a reason for hiding this comment

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

i18n for headers 😄

@aarongundel aarongundel requested a review from chrabyrd December 24, 2024 00:04
@aarongundel
Copy link
Collaborator Author

There is a second, related item to genericize the label viewer. I separated it from this PR to get commentary on patterns therein. See #161 - set to merge into this PR.

Copy link
Contributor

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

looking good, will wait for the final 👍 until the PRs based off of this one have been merged

emits("deleteLabel", tileId);
},
rejectProps: {
label: "Cancel",
Copy link
Contributor

Choose a reason for hiding this comment

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

i18n?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done in label viewer genericizing.

Copy link
Contributor

Choose a reason for hiding this comment

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

not seeing the change?

import type { AppellativeStatus } from "@/arches_lingo/types";

const { $gettext } = useGettext();
const expandedRows = ref([]);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: declare generic refs below props/emits

},
acceptProps: {
label: "Delete",
severity: "danger",
Copy link
Contributor

Choose a reason for hiding this comment

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

the severity labels exist in constants.ts

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done in label viewer genericizing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not seeing the change?

? error.message
: $gettext("Could not save the scheme standard"),
});
}

getSectionValue();
}

async function getSectionValue() {
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this could be abstracted to 2 functions to improve readability

@aarongundel aarongundel requested a review from chrabyrd December 31, 2024 05:39
* adds notes section

* fix naming nit

* wrap gettext in spans

* pr feedback

* pr feedback
@aarongundel aarongundel force-pushed the adg/146-scheme-names branch from eb4d590 to 21057da Compare January 2, 2025 22:30
Copy link
Contributor

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

looks good overall, just a few minor things ( and a few things that maybe got lost in the merge/rebase ? )

const options = await getCachedOptions();

if (options) {
await setSchemeInstance(options);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: don't need to await this

ResourceInstanceReference[] | undefined
> {
try {
const options = !textualWorkOptions.value
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: invert this

const options = textualWorkOptions.value || await getOptions();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice.

Comment on lines 80 to 81
getSectionValue();
emit(UPDATED);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would move these inside the try as these will still run even if that errors

),
});
}
emit(UPDATED);
Copy link
Contributor

Choose a reason for hiding this comment

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

same with this, it will run regardless of success

},
acceptProps: {
label: "Delete",
severity: "danger",
Copy link
Contributor

Choose a reason for hiding this comment

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

Not seeing the change?

emits("deleteLabel", tileId);
},
rejectProps: {
label: "Cancel",
Copy link
Contributor

Choose a reason for hiding this comment

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

not seeing the change?

@aarongundel
Copy link
Collaborator Author

🏓

@aarongundel aarongundel requested a review from chrabyrd January 2, 2025 22:59
Copy link
Contributor

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

I've noticed the hierarchy viewer is no longer available on this page:

Screenshot 2025-01-02 at 3 26 36 PM

Also the notes section appears to have an empty table that it didn't in previous iterations.

@aarongundel aarongundel requested a review from chrabyrd January 3, 2025 04:53
Copy link
Contributor

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

let's GOOOOO! 🚀

* Changes to allow for a single form per tile

* move section types to prep for conflict

* PR feedback

* pr feedback

* purge tab

* purge more tabs
@aarongundel aarongundel force-pushed the adg/146-scheme-names branch from 3f68847 to fa5dc08 Compare January 3, 2025 17:16
@aarongundel aarongundel merged commit 589c84d into main Jan 3, 2025
1 of 4 checks passed
@aarongundel aarongundel deleted the adg/146-scheme-names branch January 3, 2025 17:16
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.

Scheme Labels Report sections
3 participants