Skip to content
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

Merged
merged 12 commits into from
Mar 25, 2020
Merged

add permalinks #3744

merged 12 commits into from
Mar 25, 2020

Conversation

SaraVieira
Copy link
Contributor

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

@lbogdan
Copy link
Contributor

lbogdan commented Mar 24, 2020

Build for latest commit 0b88ebc failed.

@christianalfoni
Copy link
Contributor

This looks great!

❤️ the clipboard effect, would be nice to have in our current browser effect (overmind/effects/browser.ts).

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 React.useEffect we can rather execute this stuff exactly after we load the comments. The sandboxChanged action is where it loads the comments of the sandbox.

So by adding a new effect effects.router.getCommentId(), using your query logic, we can use the logic you have to run the actions in the React.useEffect. This way there are no checks "if it should run", it just runs naturally after loading the comments initially.

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 😄

Copy link
Contributor

@christianalfoni christianalfoni left a 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! 😂

Copy link
Contributor

@christianalfoni christianalfoni left a 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! 😄

packages/app/src/app/overmind/effects/router.ts Outdated Show resolved Hide resolved
@@ -162,6 +162,18 @@ export const Dialog: React.FC = () => {
<Menu.Item onSelect={() => setEditing(true)}>
Edit Comment
</Menu.Item>
<Menu.Item
onSelect={() => {
effects.browser.copyToClipboard(
Copy link
Contributor

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 😄

packages/app/src/app/pages/Sandbox/index.tsx Outdated Show resolved Hide resolved
SaraVieira and others added 4 commits March 25, 2020 12:39
Co-Authored-By: Christian Alfoni <christianalfoni@gmail.com>
Co-Authored-By: Christian Alfoni <christianalfoni@gmail.com>
Copy link
Contributor

@christianalfoni christianalfoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful ❤️ 😄

@SaraVieira SaraVieira merged commit 3742c4b into comments-glyphs Mar 25, 2020
@SaraVieira SaraVieira deleted the add-links branch March 25, 2020 13:30
christianalfoni added a commit that referenced this pull request Mar 27, 2020
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants