search

jsjoeio / speak-argentinian-spanish

speak argentinian spanish website
https://speakargentinianspanish.com/
1 stars 0 forks source link

Add search blog #29

Closed coding-in-public closed 1 year ago

coding-in-public commented 1 year ago

Okay, couple of things here:

I know this may take a while to review and I'm more than happy to adjust anything you see. I'm sure I've made some TypeScript faux pas, too, so feel free to point me in the right direction there. Hope this is at least close to what you were thinking?

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
speak-argentinian-spanish ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 4, 2023 11:22pm
jsjoeio commented 1 year ago

I don't know if you can see the build logs but here is what it says:

error   Named export 'CgSpinner' not found. The requested module 'react-icons/cg/index.esm.js' is a CommonJS module, which may not support all module.exports as named exports.
--
21:58:54.370 | CommonJS modules can always be imported via the default export, for example using:
21:58:54.370 |  
21:58:54.370 | import pkg from 'react-icons/cg/index.esm.js';
21:58:54.370 | const { CgSpinner } = pkg;
jsjoeio commented 1 year ago

Awesome! I'm signing off for the night but will look at this tomorrow when I start my work day. Excited :D

coding-in-public commented 1 year ago

Ah, yeah, weird. I just removed the react-icons package. Might as well keep the dependencies down anyhow. Substituted them for inline SVGs and then also removed the auto-focus on the blog page.

coding-in-public commented 1 year ago

Also, I sent this through right before I went to bed last night and looking at it this AM, it’s rendering oddly. For whatever reason, in local it's only updating the search results when I open/close the dev tools. I don't think I'll get time to work on this today (maybe tomorrow)? But I'm wondering if either

My plan would be to move the FuseJS processing and everything to the client and only pull that JSON file on initial page load once. Then all the rest of the rendering would be client side. Eventually, this may cause performance problems, but right now the JSON file is SUPER small and will be for some time until you add a lot of other posts.

If you have time or if you have another idea in the meantime, go for it. :)

jsjoeio commented 1 year ago

My plan would be to move the FuseJS processing and everything to the client and only pull that JSON file on initial page load once. Then all the rest of the rendering would be client side.

I like this a lot! If My preference is to go with this approach because it's straightforward then solve the performance issue if/when it happens.

If you have time or if you have another idea in the meantime, go for it. :)

Cool! May get to it tomorrow or this weekend if you don't first

coding-in-public commented 1 year ago

Hey, had 15 min tonight and I think I got everything moved over. There are a couple of TS errors that I'm sure are basic…but didn't have time to chase them down. I think everything else is working! Let me know what you think.

jsjoeio commented 1 year ago

Also there is a bit of some content shift when you enter a query vs no query. I wonder how we can fix that 🤔 Any ideas?

https://user-images.githubusercontent.com/3806031/236255831-fac6ca25-ae89-4d55-8b22-a096de8f1bb7.mov

coding-in-public commented 1 year ago

Also there is a bit of some content shift when you enter a query vs no query. I wonder how we can fix that 🤔 Any ideas?

Screen.Recording.2023-05-04.at.8.30.00.AM.mov

Ah, yeah. I think it's just the scrolling sidebar that pops in when the content overflows. I'm thinking we could do two things:

  1. Have a permanent sidebar no matter what
    • set overflow: scroll on the body OR
    • set the body section to be min-height 101vh
  2. Do some fancy css to shift the content left as if there was a sidebar (when not present) and shift back to the right when there is a sidebar. It would be hard to get it consistent since the sidebar width is different in each browser. We could probably get the body’s client width or something with JS and calculate that movement?
jsjoeio commented 1 year ago

Ah, yeah. I think it's just the scrolling sidebar that pops in when the content overflows. I'm thinking we could do two things:

Ahh right. Okay I think that's safe to save for a follow-up task then. I'm hoping there's a simpler solution like either adding the CSS you suggested or adjusting the min-height or something

coding-in-public commented 1 year ago

What's the next step here? (And this may be a dense question) but do you want me to make the suggested changes and re-push? or are you planning on making those changes and merging?

jsjoeio commented 1 year ago

What's the next step here? (And this may be a dense question) but do you want me to make the suggested changes and re-push

Ah good question! So my assumption is you:

Then re-request a review from me. Image below ICYDK:

image

Then I'll review again and we can merge!

I'll fix the layout shift in a separate PR (assuming you didn't want to, your call!)

coding-in-public commented 1 year ago

I think I did all that correctly? :)

jsjoeio commented 1 year ago

Heck yeah! Looks good from the code side. Let me commit the trailingSlash changes to your branch real quick then merge!

coding-in-public commented 1 year ago

https://user-images.githubusercontent.com/86967271/236368655-b568ef9c-6e2d-4432-a69e-218678ae32d5.mp4

🥳

jsjoeio commented 1 year ago

@coding-in-public dude you killed it! looks so professional too 🚀