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

Find media in Atom/RSS/JSON #123

Merged
merged 5 commits into from
Apr 16, 2024

Conversation

GreenTeaCake
Copy link
Contributor

@GreenTeaCake GreenTeaCake commented Mar 27, 2024

Fixes #60

Changes (please see the list of commits to get details in organized manner):

  • Add new unit tests
  • Minor refactoring of existing unit tests

Motivation

Checklist

  • Don’t rush. Check all changes in PR again.
  • Think about changing documentation.
    • If you added a script to scripts/, add a comment with a description.
    • If you added a new folder, add its description to the project’s README.md.
    • If you added config, describe how we use this tool in the config’s comment.
    • If you added something to the project’s architecture, describe it in the project’s README.md.
    • Try to focus on “why?”, not “how?”.
  • If you added a new dependency, check our requirements.
  • Think about testing
    • If you added a feature, add unit tests.
    • If you added a new state to the UI, add visual tests.
    • If you fixed the bug, think about preventing bug regression in the future.
  • If you changed web client:
    • Think about moving code to core/. What code will also be useful on other platforms?
    • Run pnpm size and check the difference in the JS bundle size. Is it relevant to the changes? Change the limit in web/.size-limit.json if necessary.
    • Think about keyboard UX. Is it easy to use the new feature with only one hand on a keyboard? Is it easy to understand what keys to press?
    • Think about HTML semantics.
    • Think about accessibility. Check a11y recommendations. Think about how screen reader users will use the tool. Is it easy to use on a screen with bad contrast?
    • Think about translations. Will
    • Think about right-to-left languages. What parts of the screen should be mirrored for Arabic or Hebrew languages?
  • If you changed the colors token in the web client:
    • Think about app loading styles inlined in index.html.
  • If you changed core/:
    • Think about making types more precise. Can you better explain data relations by type?
    • Think about conflict resolution. We don’t need some very smart changing merging; just 2 changes of the same item on different clients should not break the database. What if the user changes an item on one machine and removes it on another?
    • Think about log and storage migration.
  • If you changed English translations:
    • Change translation ID if you change the meaning of the text.

@ai
Copy link
Contributor

ai commented Mar 27, 2024

Ping me when PR will be finished

@ai
Copy link
Contributor

ai commented Apr 14, 2024

Hi. Do you plan to continue development? If you don’t have enough time, we can make task as free (but you can still continue if you will find a time).

@GreenTeaCake
Copy link
Contributor Author

Hi. Do you plan to continue development? If you don’t have enough time, we can make task as free (but you can still continue if you will find a time).

Hello! I'm really sorry for the delay. Will try to bring the code for review or ask to un-assign tonight.

@GreenTeaCake GreenTeaCake force-pushed the find-media-in-atom-rss-feeds branch 4 times, most recently from 784ff68 to b413569 Compare April 15, 2024 13:20
@GreenTeaCake GreenTeaCake marked this pull request as ready for review April 15, 2024 13:24
@GreenTeaCake GreenTeaCake requested a review from ai April 15, 2024 15:29
core/test/loader/rss.test.ts Outdated Show resolved Hide resolved
core/loader/atom.ts Outdated Show resolved Hide resolved
core/test/loader/atom.test.ts Outdated Show resolved Hide resolved
core/test/loader/atom.test.ts Outdated Show resolved Hide resolved
core/loader/rss.ts Outdated Show resolved Hide resolved
Externalise re-usable logic to create RSS/Atom test response.
@ai
Copy link
Contributor

ai commented Apr 16, 2024

Sorry, another loader changes. Can you add the media search to core/loader/json-feed.ts too?

The last one, I promise :).

@ai
Copy link
Contributor

ai commented Apr 16, 2024

Everything else looks good. Great work.

@ai
Copy link
Contributor

ai commented Apr 16, 2024

Just fix ESLint warnings by running pnpm eslint --fix .

Tests for parsing media (image) links from RSS and Atom posts.
@GreenTeaCake
Copy link
Contributor Author

Sorry, another loader changes. Can you add the media search to core/loader/json-feed.ts too?

The last one, I promise :).

Sure thing. NP. Will add parsing tonight

core/loader/json-feed.ts Outdated Show resolved Hide resolved
core/loader/json-feed.ts Outdated Show resolved Hide resolved
core/loader/json-feed.ts Outdated Show resolved Hide resolved
core/loader/rss.ts Outdated Show resolved Hide resolved
core/loader/rss.ts Outdated Show resolved Hide resolved
@GreenTeaCake GreenTeaCake changed the title Find media in Atom/RSS Find media in Atom/RSS/JSON Apr 16, 2024
@GreenTeaCake GreenTeaCake requested a review from ai April 16, 2024 20:27
@ai ai merged commit 5964229 into hplush:main Apr 16, 2024
3 checks passed
@ai
Copy link
Contributor

ai commented Apr 16, 2024

Thanks! Amazing work.

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.

Find media in Atom/RSS
2 participants