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

Rough out scheme list #122

Merged
merged 10 commits into from
Dec 9, 2024
Merged

Rough out scheme list #122

merged 10 commits into from
Dec 9, 2024

Conversation

johnatawnclementawn
Copy link
Member

@johnatawnclementawn johnatawnclementawn commented Nov 6, 2024

Depends on #86 & #104

@johnatawnclementawn johnatawnclementawn marked this pull request as ready for review November 6, 2024 21: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.

looks pretty good! I couldn't get it working for me unfortunately, I think because of the SchemeBoxes logic. Happy to give it another pass once I can get it running 👍

@chrabyrd chrabyrd self-assigned this Nov 15, 2024
Base automatically changed from jtw/prototype-serializers to jtw/new-tree-front-end November 20, 2024 18:49
@johnatawnclementawn johnatawnclementawn force-pushed the jmc/16_schemes_list branch 2 times, most recently from 1e9c49c to b64b55d Compare November 21, 2024 16:13
@johnatawnclementawn
Copy link
Member Author

now depends on archesproject/arches-vue-utils#6

@chrabyrd
Copy link
Contributor

chrabyrd commented Dec 3, 2024

👋 @johnatawnclementawn ! Can you resolve the merge conflicts?

Also unless I'm mistaken, this has a lot of leakage from jtw/new-tree-front-end ?

Base automatically changed from jtw/new-tree-front-end to main December 3, 2024 21:57
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! Just some minor things

arches_lingo/src/arches_lingo/routes.ts Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
arches_lingo/src/arches_lingo/pages/SchemeList.vue Outdated Show resolved Hide resolved
arches_lingo/src/arches_lingo/pages/SchemeList.vue Outdated Show resolved Hide resolved
@johnatawnclementawn
Copy link
Member Author

@chrabyrd since we talked about fall-back values, flagging these lines

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 1 question.


onMounted(async () => {
schemes.value = await fetchSchemes();
schemes.value.unshift({
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting pattern. An equally valid pattern would be to manually create a list item for this entry. However I like this just fine 👍

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Looks great, just a couple things I think we might harden.

arches_lingo/src/arches_lingo/pages/SchemeList.vue Outdated Show resolved Hide resolved
arches_lingo/src/arches_lingo/pages/SchemeList.vue Outdated Show resolved Hide resolved
onMounted(async () => {
schemes.value = await fetchSchemes();
schemes.value.unshift({
resourceinstanceid: "placeholder-id",
Copy link
Member

Choose a reason for hiding this comment

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

Clicking this populates the front-end route with /placeholder-id, and then refreshing issues a request for that route which fails because placeholder-id is not a uuid. We should harden for this somehow, but maybe without necessarily creating dummy data on the backend that we have to filter out everywhere else (which is what I like about what you're doing here!)

Copy link
Member Author

@johnatawnclementawn johnatawnclementawn Dec 9, 2024

Choose a reason for hiding this comment

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

I wasn't exactly sure what to do, which is why I ended up punting with placeholder-id... My thought was that a PR for adding the scheme-editor and it's corresponding route would be coming hot on the heels of this, and we could just swap placeholder-id for the new scheme route.

As you point out, it's probably not desirable to have a dummy uuid in lieu of string placeholder-id because we'd need to handle that on the back-end.

Alternatively, I could mint the new scheme resourceid here - but I see that as less desirable since it would create a new uuid each time /schemes is visited, even if creating a new scheme is not the intention of the user.

All that to say, I'm not exactly sure what to do about this

Copy link
Member

Choose a reason for hiding this comment

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

Ah, got it. If we're about to swap in a new route for the scheme editor then all is good 👍

onMounted(async () => {
schemes.value = await fetchSchemes();
schemes.value.unshift({
resourceinstanceid: "placeholder-id",
Copy link
Member

Choose a reason for hiding this comment

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

Ah, got it. If we're about to swap in a new route for the scheme editor then all is good 👍

@johnatawnclementawn johnatawnclementawn linked an issue Dec 9, 2024 that may be closed by this pull request
@johnatawnclementawn johnatawnclementawn merged commit 6dd1d2b into main Dec 9, 2024
4 of 6 checks passed
@johnatawnclementawn johnatawnclementawn deleted the jmc/16_schemes_list branch December 9, 2024 19:14
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.

Add schemes to scheme list page
3 participants