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

added useQuote hook #88

Closed
wants to merge 2 commits into from
Closed

added useQuote hook #88

wants to merge 2 commits into from

Conversation

NancyAanchal
Copy link
Contributor

I tried a bunch of methods but this is the only one that worked. it works but it prints the quote 2 times, I couldn't figure out why.
also moved tajpuriyaquotes.txt to src/quotes/ and renamed it 'tajpuriya.txt' cuz otherwise the path of it from hook would be complex.

const fetcher = (url: string) => fetch(import.meta.env.VITE_NEPALBHASA_API_URL + url, {
}).then(r => r.json())
const useNewari = (props: Omit<DictionaryProps, "language">) => {
let { data, error, isLoading } = useSWR(
Copy link
Member

Choose a reason for hiding this comment

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

looks like your branch might be out of date - can you merge in main into this branch (or rebase) ?

Copy link
Member

@christikaes christikaes left a comment

Choose a reason for hiding this comment

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

This looks good! I would just recommend a small change to switch to csv format instead of txt

Copy link
Member

Choose a reason for hiding this comment

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

Can we reformat this into .csv format? I think that will make it easier to edit - eg open in excel etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'small' change? well wouldn't it mean reshaping the whole code 😭? I will try to do that tho!

Copy link
Member

Choose a reason for hiding this comment

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

you did it! it looks good, just looks like you have a little bit of stale logic here

Copy link
Member

Choose a reason for hiding this comment

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

Yeah our bad, we should have made sure it was a CSV beforehand.

Copy link

cloudflare-workers-and-pages bot commented Jul 10, 2024

Deploying nepalingo with  Cloudflare Pages  Cloudflare Pages

Latest commit: a7991a3
Status: ✅  Deploy successful!
Preview URL: https://0afb0346.nepalingo.pages.dev
Branch Preview URL: https://quote-hook.nepalingo.pages.dev

View logs

return {
text: text ? text.trim() : "",
translation: translation ? translation.trim() : "",
text: text ? text.trim().replace(/(^"|"$)/g, "") : "",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this anymore right? there are no | separators?

Copy link
Member

@christikaes christikaes left a comment

Choose a reason for hiding this comment

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

A few small cleanup left

Copy link
Member

Choose a reason for hiding this comment

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

you did it! it looks good, just looks like you have a little bit of stale logic here

@christikaes
Copy link
Member

Closing because these changes got merged in the #91 PR

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.

3 participants