benawad / codeponder

Marketplace for Code Reviews
279 stars 49 forks source link

Some doubts about my recent PRs #113

Open paean99 opened 5 years ago

paean99 commented 5 years ago

The empty style sheet that was added to solve the nextjs bug seem to not work very well. I experienced several time, that when i click in the first link and right after a cold start of codeponder, that it can take several second for the page to change. It happens only the first time and not every restart. I have not found a pattern yet, but i believe that it may be related to the state of the nextjs build.

Another thing that seem to not be working in the best way: Some times after the codeponder starts and also only the first code page to be accessed, the syntax highlighting is not working. Just refreshing the page solves it, but i am not sure why it is happening. Probably also because of nextjs ssr.

A feature is missing from the line number input, they cannot reset themselves. At this moment the only way is through the mouse clicks. The easiest and simpler solution is adding a reset button to the form, but i am not sure what you envision for this feature. One alternative would be to put one of the input to '0'. It shouldn't be problematic in any way to implement. One feature i added and probably not mentioned is that it is impossible to input numbers lesser (negative) or bigger than the number of lines in the code (need more work, since the code line lenght is calculated another time, even though react renderer does it already).

As for the line number mouse selection, i tried to get as near as i could to the github mouse selection functionality. But although i tried to let the code be as simple and straightforward as possible, i never explained the behavior. So i will take this space to elaborate a little.

I have been thinking that a "code ui component" (for storybook) should be the next iteration. It could be reused even in the comment and styled in a more controlled way. But after seeing https://github.com/benawad/codeponder/issues/109 (exited to see it released or at least some variant of it), i think that it should wait for the PR. Because it would imply to move the react renderer package to packages/ui to avoid the same problem as styled components and it would surely create to much code conflict with @onemem work.

benawad commented 5 years ago

I've noticed weird stuff happening too and wasn't sure if it's a bug with the css import, ssr, or how we load prism. I wonder if this will show up in the production build.

I'm not exactly sure how I want to handle the line numbers yet, I think I will leave that last.

I think waiting is a good idea and I think it's ok to put the shareable component in the web package instead of the ui one