-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
add permalinks #3744
add permalinks #3744
Conversation
Build for latest commit 0b88ebc failed. |
This looks great! ❤️ the clipboard effect, would be nice to have in our current To get more insight into what is happening when we load a Sandbox I want to suggest an alternative approach, but you can evaluate if you want to do that refactor or not @SaraVieira 😄 Instead of using a So by adding a new effect This just generally gives us more control of what and when things happens when we load a Sandbox and we avoid the risk of the React effect running the logic when it should not... which would be difficult to debug. Again, I do not really see any risk with the implementation so it is totally up to you if you want to do this change 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! 👍 😄
Just some minor stuff. Sorry if I did not consider some scenario with the suggestion on the bounds, but shits getting complicated! 😂
packages/app/src/app/pages/Sandbox/Editor/Workspace/screens/Comments/Comment.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this is final stuff! 😄
@@ -162,6 +162,18 @@ export const Dialog: React.FC = () => { | |||
<Menu.Item onSelect={() => setEditing(true)}> | |||
Edit Comment | |||
</Menu.Item> | |||
<Menu.Item | |||
onSelect={() => { | |||
effects.browser.copyToClipboard( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I have a suggestion here.
You can create an action in comments
called like: copyPermalinkToClipboard
, where you pass the commentId
. Inside that action you do what you do here, run the two effects.
The great thing now is that both Dialog
and Comment
will just call this action and we avoid duplicate logic 😄
Co-Authored-By: Christian Alfoni <christianalfoni@gmail.com>
Co-Authored-By: Christian Alfoni <christianalfoni@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful ❤️ 😄
* initial stuff * initial glyph implementation * optimistic transposing * add active class to glyphs * WIP: add comment from context * handling optimistic thread * multi comments on line * favor active over multi icon * add numbers * copy changes from #3729 * remove trigger check * remove react-draggable coz we have framer motino * open file and scroll to position * refactored most, continuing on laptop... * fix replies * glyph likes themes * oops that not supposed to be there * extract avatarblock * move edit comment out * use edit content in reply * add dialog positioning * always render even if position is not known * better fallback position * remove unused ref * add polling mechanism to active comment * change the fallback just a little more * optimistically set position * fix checking id in comment glyph boundary as well * remove optimistic trigger * improve moving comments * use currentTarget instead * set placeholder + animate from left in fallback * move position logic into a funktion * animate based on _file change_ * rename optimistic to newComment * add permalinks (#3744) * add permalinks * remove query from effect * move copy to clipboard to effect * add defaults * create action for commenturl * yaay * Apply suggestions from code review Co-Authored-By: Christian Alfoni <christianalfoni@gmail.com> * Update packages/app/src/app/pages/Sandbox/index.tsx Co-Authored-By: Christian Alfoni <christianalfoni@gmail.com> * make action Co-authored-by: Christian Alfoni <christianalfoni@gmail.com> * fixed add comment position and decorate line * redesign new comment * fix ... icon * pull out new comment component * rename to AddComment duh * move add reply to a component * pull out comment header * pull out comment body into a component * close dialog is lame * but i am lamer * leave a comment before i forget * add comment glyph * some fixes * window collisions - first draft * cheeky scale guessing * found the missing 48px :P * dont need to track animation state now * always div for unknown semantics * fix comment current target trigger * omg strings! * fix animations on comments * improve handling optimistic * use animate controller * add comments * Add times and move file name (#3752) Co-authored-by: Christian Alfoni <christianalfoni@gmail.com> * fix issue with moving from optimistic to existing comment * refactor to initial and end position * ensure we actually have a comments state for the sandbox * fix multi popup * auto focus on creating new comment * add permission checks on comments * drag handle!!! * improve dragging * style multi comments (#3763) * style multi comments * use element * update lookup * fix ot typing * fixed types and defaulting to comments * fix typing * last fixes Co-authored-by: Sara Vieira <hey@iamsaravieira.com> Co-authored-by: siddharthkp <siddharth.kshetrapal@gmail.com>
Adds permalinks for comments
Linking to:
http://localhost:3000/s/twilight-platform-fqnmj?comment=0ab2c879-de32-4227-9a79-fc5899f9d72f
Opens the comments sidebar and sets that comment as the selected one opening it