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

Cache Overhaul for OpenSeadragon (reviewed). #289

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Aiosa
Copy link
Contributor

@Aiosa Aiosa commented Nov 22, 2024

Copy link
Member

@iangilman iangilman left a comment

Choose a reason for hiding this comment

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

This is awesome! Thank you for putting all of this documentation together.

There are some places where there are some English grammatical oddities... Your English is excellent, but still there are some things I would change to make it read more smoothly. Perhaps I should do an editing pass to touch up some of those… seems easier than pointing them all out via review. What do you think?

www/migration-v5.html Outdated Show resolved Hide resolved
www/migration-v5.html Outdated Show resolved Hide resolved
www/migration-v5.html Show resolved Hide resolved
www/data-plugins.html Outdated Show resolved Hide resolved
www/data-plugins.html Outdated Show resolved Hide resolved
www/data-plugins.html Outdated Show resolved Hide resolved
www/data-types.html Outdated Show resolved Hide resolved
@Aiosa
Copy link
Contributor Author

Aiosa commented Dec 2, 2024

Perhaps I should do an editing pass to touch up some of those

Feel free to edit any content. I am well aware I have problems to come up with easily understandable text 😄

…e description. Rename everywhere docs page data-plugins -> data-modification.
@iangilman
Copy link
Member

Cool; I'll do a pass on this at some point. Otherwise I think it's good to go. We won't merge it until we release 6.0.0, of course.

www/data-types.html Outdated Show resolved Hide resolved
like signature). Overriding cache key generation may or might not be
necessary.
Upon data submission, you must provide a data type you submit, and
optionally provide type convertors in case your format is not supported
Copy link
Member

Choose a reason for hiding this comment

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

I've just noticed this (despite having seen the code before many times): the standard English spelling is "converter". Evidently "convertor" is also legal, but much less common. Perhaps we should rename the feature to align with common English spelling? Apologies for not noticing this sooner!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also in the whole codebase? I think variables/classes have the name too.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think so. Again, apologies for not catching it sooner!

@Aiosa
Copy link
Contributor Author

Aiosa commented Jan 30, 2025

@iangilman I've added docs on drawer design as well, in case you want to check that out too.

@iangilman
Copy link
Member

@Aiosa Awesome. I've taken a look and made a new PR. Thank you for making that document!

@pearcetm You might take a look at the drawer design document and see if you have any comments.

Wonderful to have this stuff documented in this way!

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.

2 participants