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 note editor (#177) #181

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

Adds note editor (#177) #181

wants to merge 1 commit into from

Conversation

aarongundel
Copy link
Collaborator

No description provided.

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.

overall looks good just 1 small thing

@aarongundel aarongundel requested a review from chrabyrd January 14, 2025 21:14
@aarongundel aarongundel linked an issue Jan 15, 2025 that may be closed by this pull request
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! One minor question that doesn't really need to be dealt with here

@@ -16,6 +16,7 @@
api_scheme_label_create="{% url 'api-scheme-label-create' %}"
api_scheme_note='(resourceid)=>{return "{% url "api-scheme-note" "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" %}".replace("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", resourceid)}'
api_scheme_note_tile='(resourceid, tileid)=>{return "{% url "api-scheme-note-tile" "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaab" %}".replace("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", resourceid).replace("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaab", tileid)}'
api_scheme_note_create="{% url 'api-scheme-note-create' %}"
Copy link
Contributor

Choose a reason for hiding this comment

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

should this just fall under the api_scheme_note as a different verb?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like it, but that doesn't actually work. The difference is in pythonic_models.py - you'll see there are different views used for these routes. The ListCreateView is necessary here because we're dealing with a list of tiles. If I was to try a patch or a put against api_scheme_note (which uses RetrieveUpdateDestroyAPIView) I'd blow away the rest of the list.

Base automatically changed from jmc/147_scheme_label_editor to main January 17, 2025 23:12
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 notes editor
2 participants