plebbit / plebchan

A GUI for plebbit similar to 4chan
https://plebchan.eth.limo
GNU General Public License v2.0
21 stars 8 forks source link

Add refresh button in catalog views #157

Closed plebeius-eth closed 1 year ago

plebeius-eth commented 1 year ago

It must be identical to the return button, placed to its right, using the same styles (yotsuba, yotsuba b, etc.), in Catalog.jsx, AllCatalog.jsx and SubscriptionsCatalog.jsx.

doxometrist commented 1 year ago

hi is it ok for me to work on this?

plebeius-eth commented 1 year ago

hi is it ok for me to work on this?

sure, just assigned you to it

doxometrist commented 1 year ago

started, exploring the details of the views in question, expecting to create pull request tomorrow

doxometrist commented 1 year ago

@plebeius-eth should the button be also in the Description.jsx and Thread.jsx?

plebeius-eth commented 1 year ago

@plebeius-eth should the button be also in the Description.jsx and Thread.jsx?

yes actually that's a good idea, but it should still say "[Refresh]", not "[Update]" because the plebbit API only updates automatically, but a page refresh can still be useful to force state updates

doxometrist commented 1 year ago

ok it's working resolving some merge conflicts rn, will push to dev soon

doxometrist commented 1 year ago

out of curiosity - why move away from virtuosogrid into infinitescroll?

doxometrist commented 1 year ago

ok it's merged but can't push for auth reasons, I have my ssh key agent running, maybe it's with the repo config?

plebeius-eth commented 1 year ago

out of curiosity - why move away from virtuosogrid into infinitescroll?

oh you mean before the latest commit that moved from infinitescroll to virtuosogrid? Virtuoso calculates the scrolling measuring the content in its data prop, so the data must be static otherwise it will glitch, which is what was happening with media since it would load dynamically. To fix this, Rinse and Esteban added comment.linkWidth and comment.linkHeight to the API, so all posts since then have these two properties that tell Virtuoso the size before loading the feed. To fix the glitch for all comments without those proprieties (esp. coming from the older API) I forced the size statically to always be a square: e486b702da02efbf75b922c3483272846c6ee5c2

then I could finally move from infinitescroll to virtuosogrid: 62417720160ec6bdb1d5874bfe0d5169a6a2f9e0

ok it's merged but can't push for auth reasons, I have my ssh key agent running, maybe it's with the repo config?

yes you can open a pull request so I can review the code before merging

doxometrist commented 1 year ago

to open the pull request I'd need to have a branch on the remote it seems. and I cannot do that either. not sure how to proceed atm

doxometrist commented 1 year ago

here's the error: Pull request creation failed. Validation failed: must be a collaborator

plebeius-eth commented 1 year ago

you have to fork and clone the repo, so you can commit on your own branch and then open a pull request to merge it with the main repo, like: https://github.com/plebbit/plebchan/pull/161 I can't assign new collaborators yet (ie direct write access)

doxometrist commented 1 year ago

ah I see, thanks

doxometrist commented 1 year ago

will do this tomorrow

doxometrist commented 1 year ago

ok making a pull request now, tbh might consider adding a button to the board as well

doxometrist commented 1 year ago

my bad, misclicked and went into master, let me rectify this

doxometrist commented 1 year ago

ok it should be alright now

plebeius-eth commented 1 year ago

I tried to merge this but I had to revert, it's full of conflicts. It seems like you used old code from master, and then attempted to commit it into dev. You can see the huge diffs when comparing, for example you're still using InfiniteScroll.

To avoid this problem: as time passes when you code for a branch, and new commits come in, you should always git pull to to update the branch, and adapt your code along the way, in order to minimize the number of possible conflicts later on.

So now if you want you'd have to first clone either dev or master, be sure to pull the latest code from it, and then commit your changes to that updated code, and open the pull request for the same branch you're committing to. This way conflicts will be minimized and I'll fix them if any.

doxometrist commented 1 year ago

ah I see, sorry for the mess. will fix tomorrow

plebeius-eth commented 1 year ago

cool thanks

doxometrist commented 1 year ago

there's errors, working on them now

doxometrist commented 1 year ago

Now I'm trying to put it as a component, which should be easy but the imports aren't working right, this might be because of that babel issue. have you dealt with this before?

`plebchan$ yarn build yarn run v1.22.19 $ cross-env PUBLIC_URL=./ GENERATE_SOURCEMAP=false react-scripts build Creating an optimized production build... One of your dependencies, babel-preset-react-app, is importing the "@babel/plugin-proposal-private-property-in-object" package without declaring it in its dependencies. This is currently working because "@babel/plugin-proposal-private-property-in-object" is already in your node_modules folder for unrelated reasons, but it may break at any time.

babel-preset-react-app is part of the create-react-app project, which is not maintianed anymore. It is thus unlikely that this bug will ever be fixed. Add "@babel/plugin-proposal-private-property-in-object" to your devDependencies to work around this error. This will make this message go away.

Failed to compile.

Module not found: Error: Can't resolve '../buttons/RefreshButton' in '/home/doxometrist/write-forks/plebchan/src/components/views'

error Command failed with exit code 1. info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.`

plebeius-eth commented 1 year ago

you can do yarn start to test on localhost

that babel message is just a warning, you can ignore it, the reason why it's failing to compile is because the import statement is wrong (../buttons/RefreshButton), try import RefreshButton from '../../buttons/RefreshButton';

doxometrist commented 1 year ago

yes I did use yarn start the same error was happening. ah the imports lol

doxometrist commented 1 year ago

I returned to this today and the import is erroneous wherever I put the file. in src/components/buttons, in views directly. idk, maybe there's some global setting with the imports that prevents this being right

plebeius-eth commented 1 year ago

I returned to this today and the import is erroneous wherever I put the file. in src/components/buttons, in views directly. idk, maybe there's some global setting with the imports that prevents this being right

Check this out https://github.com/plebbit/plebchan/pull/161#issuecomment-1687669349 I think yours is a very similar situation. It seems to me that the best thing to do in these cases is to fork the repo again, starting a brand new branch, then applying your changes again in the updated code. This is a much faster process than resolving conflicts generated from very old code. To avoid this in the future you should make sure to always do git pull whenever there's new code to pull, and keep coding as you do that, in order to minimize conflicts.

Also might help: in the repo, do rm -rf yarn.lock node_modules and then do yarn

doxometrist commented 1 year ago

no, I already did that. the imports don't work right even if I pulled before

plebeius-eth commented 1 year ago

Not sure why that is, nobody else is having problems with imports and there's no global setting for them. If you fork master again and start a brand new branch for it you shouldn't encounter any problem whatsoever: re-apply your existing code to the new branch, while you code keep fetching the origin to pull newer commits from master if any, in order to keep your branch up to date minimizing conflicts later on, and when you are finished open a new pull request for this newer branch.

plebeius-eth commented 1 year ago

Closing this, it's not needed anymore, mainly because of this UI change: 8c6ea9f776936981e296f83c681c97cd70e98d8a and this new toast which is a good alternative for this issue: 06803548f1c54e5c6900a4a95e29b096a9099376