search

withastro / houston-discord

MIT License
10 stars 5 forks source link

Add "Files" button on `/ptal` command #82

Closed thomasbnt closed 9 months ago

thomasbnt commented 9 months ago

Why ?

Because it's more simple and fast to see files directly for review. At this moment, for an review, we need to click on the only available button "View on GitHub", wait for it to load, and click on Files.

Information

The emoji at line 222 is 📁 (a opened folder)

Preview

Preview of the /ptal command with Files button

thomasbnt commented 9 months ago

Small mistake in actually adding the component. I'm curious how you got the preview image as this code would duplicate the github link

For this question, I edited the HTML page for this preview. I haven't tested the robot, but I know about discord.js and builder modules. If ever, it's to be tested, but if the similar component above and below works without a hitch, then this one should work too. However, I tested it on one of my robots to integrate the folder emoji. It displays just as it does in the preview.

mandar1jn commented 9 months ago

Small mistake in actually adding the component. I'm curious how you got the preview image as this code would duplicate the github link

For this question, I edited the HTML page for this preview. I haven't tested the robot, but I know about discord.js and builder modules. If ever, it's to be tested, but if the similar component above and below works without a hitch, then this one should work too. However, I tested it on one of my robots to integrate the folder emoji. It displays just as it does in the preview.

Makes sense. It looks like it should all work. I mostly want to test refreshing since that implementation is... not the most fault resilient

mandar1jn commented 9 months ago

Just like I guessed. This behavior currently breaks reloading. I'm going to take a bit of time to rewrite the reloading logic and this should then be good to be merged on top of that

thomasbnt commented 9 months ago

Just like I guessed. This behavior currently breaks reloading. I'm going to take a bit of time to rewrite the reloading logic and this should then be good to be merged on top of that

Good!

mandar1jn commented 9 months ago

@thomasbnt could you please rebase onto main? I just merged #84 which should alleviate the reload issues, clearing this up to merge

thomasbnt commented 9 months ago

@thomasbnt could you please rebase onto main? I just merged #84 which should alleviate the reload issues, clearing this up to merge

yeep just merged!

mandar1jn commented 9 months ago

Great. I’ll just test it out quickly tomorrow to make sure reloading works now and then I’ll merge it

thomasbnt commented 9 months ago

Everything works now

Awesome thanks!