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

3box threads #2146

Merged
merged 8 commits into from
Oct 13, 2020
Merged

3box threads #2146

merged 8 commits into from
Oct 13, 2020

Conversation

dkent600
Copy link
Contributor

@dkent600 dkent600 commented Sep 23, 2020

Resolves: #1553

Note 3box does not have out-of-the-3box support for editing or replying to comments. We can implement those ourselves, but as a separate PR.

@dkent600 dkent600 marked this pull request as draft September 23, 2020 20:33
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 September 23, 2020 20:37 Inactive
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 September 24, 2020 14:55 Inactive
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 September 24, 2020 18:33 Inactive
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 September 25, 2020 03:52 Inactive
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 September 25, 2020 14:35 Inactive
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 September 30, 2020 14:12 Inactive
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 September 30, 2020 19:38 Inactive
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 October 1, 2020 10:15 Inactive
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 October 1, 2020 14:06 Inactive
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 October 1, 2020 14:51 Inactive
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 October 1, 2020 20:08 Inactive
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 October 1, 2020 20:32 Inactive
@roienatan
Copy link
Contributor

roienatan commented Oct 2, 2020

  1. After submitting a comment the content inside the text area should disappear (need to think about it since as I understood there is no option to edit comments after submitting)
  2. I would change the trash color to bright red
  3. No need the "Please confirm" tooltip when hovering the trash icon
  4. Feels like the Submit button should placed in the right (like the rest of the submit/ok buttons across the project) and maybe green color like the Join Conversation button
  5. A grey thin line between each comment would be nice (see attached image)

Screen Shot 2020-10-02 at 15 43 08

6. The orange question mark is unnecessary there. I guess it's part of the text component - maybe need to add prop to the text component which we can control whether to show the orange question mark or not.
  1. The Yes and No buttons in the delete modal are not styled as the rest of the buttons in Alchemy (I think they have unnecessary border?)

  2. Try placing the comment date under the name - I think it looks better.

Screen Shot 2020-10-02 at 16 01 30

@@ -122,6 +123,7 @@
"i18next": "^19.6.3",
"immutability-helper": "2.6.4",
"interweave": "^11.1.0",
"levelup": "^4.4.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this is used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

work-around for a 3box bug. before submitting this for review (note it is still a draft) I plan to check whether they have fixed the bug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a query in with 3box to discover whether this work-around is still necessary

@@ -3,6 +3,7 @@ export const ETHDENVER_OPTIMIZATION = true;
// if this is true, we do get the contractInfos from a locally stored file in ./data instead of from the subgraph
export const USE_CONTRACTINFOS_CACHE = false;
export const GRAPH_POLL_INTERVAL = 30000;
export const THREEBOXTHREADSMODERATOR = "0xd50fc49ff389558d23a76Cf246dA147FF53D8Df8";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it some private key? If so it should be as an ENV_VAR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, that is the address of the moderator, is meant to be public

@orenyodfat
Copy link
Contributor

remove the warning.. there is no need for alchemy 2

@dkent600
Copy link
Contributor Author

dkent600 commented Oct 6, 2020

@roienatan

  1. After submitting a comment the content inside the text area should disappear (need to think about it since as I understood there is no option to edit comments after submitting)

For me this is not a big deal either way. I chose the current behavior because 1) I am never comfortable deleting a user's data unless there is a really good reason, and 2) we don't have a way to edit comments once submitted. So to change a comment one has to delete and resubmit

  1. I would change the trash color to bright red

Why? Red doesn't look very good in this color palette, and is a color that has a tendency to create tension. Since we prompt to confirm the deletion, I see no need for anything alarming.

  1. No need the "Please confirm" tooltip when hovering the trash icon

I changed it to "Delete this comment...", which I think is needed so people know what will happen if they click it. The "..." is standard in computer UX to indicate that a further prompt will follow.

  1. Feels like the Submit button should placed in the right (like the rest of the submit/ok buttons across the project) and maybe green color like the Join Conversation button

I don't agree with either suggestion, especially the button position. 1) this isn't a modal popup, and the width of the page is significantly great, 2) the user's eyes, typing from left to right with left-justified text, are always going to be biased to viewing the left side, 3) the grey button color matches the grey palette of the markdown component, thus helping the user to comfortably and intuitively parse the markdown component and the button as a functional group amidst the surrounding clutter.

  1. A grey thin line between each comment would be nice

I thought of that, however, Discus doesn't do it, google search doesn't do it, I feel that there are already plenty of visual cues enabling the user to comfortably parse the page without adding clutter to it.

  1. The orange question mark is unnecessary there. I guess it's part of the text component - maybe need to add prop to the text component which we can control whether to show the orange question mark or not.

This help button is our standard where ever we have the ability to compose markdown. I would not remove or change it unless we remove or change it everywhere, and that should be as a separate PR.

  1. The Yes and No buttons in the delete modal are not styled as the rest of the buttons in Alchemy (I think they have unnecessary border?)

I only see a problem with the No button, so I removed the border. Otherwise these buttons appear the same way whenever we have a SimpleMessagePopup (which until now has only been with an OK button). If we want to improve it really ought to be a separate PR.

  1. Try placing the comment date under the name - I think it looks better.

Done:
image

@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 October 6, 2020 19:51 Inactive
@dkent600 dkent600 had a problem deploying to alchemy-add3boxthreads-dpi9dn1 October 7, 2020 18:25 Failure
@dkent600 dkent600 had a problem deploying to alchemy-add3boxthreads-dpi9dn1 October 7, 2020 20:20 Failure
@dkent600 dkent600 had a problem deploying to alchemy-add3boxthreads-dpi9dn1 October 7, 2020 21:08 Failure
@dkent600 dkent600 had a problem deploying to alchemy-add3boxthreads-dpi9dn1 October 7, 2020 21:09 Failure
@dkent600 dkent600 had a problem deploying to alchemy-add3boxthreads-dpi9dn1 October 7, 2020 22:06 Failure
@dkent600 dkent600 had a problem deploying to alchemy-add3boxthreads-dpi9dn1 October 8, 2020 00:34 Failure
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 October 8, 2020 01:12 Inactive
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 October 8, 2020 10:19 Inactive
roienatan and others added 4 commits October 12, 2020 08:48
* Updated arc.js, test env and subgraph

* join and quit --> join to match new arc.js

* updated doc to Join instead of JoinAndQuit
* upgrade provider packages

* update package-lock

* remove webpack from dependencies

* package-lock

* lint

* refactor, fix bugs

* better console info on errors

* clean up account profile loguc

* remove unused webReducer stuff

* merge conflict errors

* added some i18next

* upgrade provider packages

* remove webpack from dependencies

* package-lock

* refactor, fix bugs

* better console info on errors

* clean up account profile loguc

* merge conflict errors

* added some i18next

* package-lock

* 3box fix for identity-wallet

* fix bug in SAVE_THREEBOX

* revert to 3box 1.20.3

* fix error in follow

* remove Try Edit button

* clean up canEdit logic

* remove console.log statements

* fix merge conflict

* fix compile error

* Update translation.json

* update package-lock
update package-lock

optimizations, upgrade 3box to 1.22.0

levelup update levelup to work-around 3box issue

lots more functionality

more good stuff

more grooviness!

add markdown ability

improve error messages a bit

help text, i18next

fix update after new comment

really fix new post subscription

add delete post button

compile error

translations, tooltips

fix logging out issues

allow resizing of markdown editor

conform prompt before deletion

fetching spinner

PR comments, remove Disqus from DAO landing page

create ThreeBoxThreads component

clone threads across the app
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 October 12, 2020 12:52 Inactive
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 October 12, 2020 12:57 Inactive
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 October 12, 2020 16:26 Inactive
@dkent600 dkent600 marked this pull request as ready for review October 12, 2020 16:42
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 October 13, 2020 00:09 Inactive
<Tooltip placement="top" trigger={["hover"]} overlay={i18next.t(this.state.threeboxPosts?.length ? "Enable3BoxInteractions" : "CreateFirst3BoxPost")}>
<a onClick={this.startDiscussion}>{i18next.t(this.state.threeboxPosts?.length ? "JoinTheConversation" : "StartAConversation")}{this.state.joiningThread ? <img className={css.loading} src="/assets/images/Icon/buttonLoadingWhite.gif" /> : ""}</a>
</Tooltip>
: ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please not use the pattern of empty quotation marks? Instead you can use &&, for example:
{ this.state.joinedThread && <div>your content</div> }

I find this empty quotation marks pattern confusing and I'm trying to eliminate this across Alchemy.
I would use the ternary operation only when I have something to show if the other condition met, otherwise && is much more cleaner.

Please change on the other places also.

Copy link
Contributor Author

@dkent600 dkent600 Oct 13, 2020

Choose a reason for hiding this comment

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

frankly I find your logic much more confusing because I have to understand how the boolean expression is evaluated left to right coupled with the fact that the boolean value of the <div> is totally irrelevant and that the final result of the whole expression is never actually evaluated, suggesting to me that some linters might complain and compilers/optimizers might motivated to remove the entire block. You are relying on a side effect which is not a good design pattern.

Copy link
Contributor Author

@dkent600 dkent600 Oct 13, 2020

Choose a reason for hiding this comment

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

Further, asking me to change this logic risks creating bugs forcing me to test everything all over again. Would have been better to ask for this sort of thing in advance.

Copy link
Contributor Author

@dkent600 dkent600 Oct 13, 2020

Choose a reason for hiding this comment

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

On further reflection I realize that contrary to what I asserted above, the final result of the whole expression actually is evaluated, but again as an unusual side effect. But that realization doesn't much alter the fact that the expression has taken me a while to fully grok, a lot longer than the terniary, so I'm still not very compelled to agree that this is a simpler way of doing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I'm just dense. Bottom line is I'd prefer not have to retest all this code for this late request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, react sucks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you beg me I'll do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK OK I did it in the one file, but I'll go on record as saying that for me the new logic is less intuitive/clear than terniaries.

And that react sucks in any case.

}

a {
color: #0071ff;
Copy link
Contributor

Choose a reason for hiding this comment

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

Global variable for color, I guess we already use this color in the globals.

Copy link
Contributor Author

@dkent600 dkent600 Oct 13, 2020

Choose a reason for hiding this comment

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

ok, I updated 26 files with color variables I've used in this PR. No way I'm testing all that though...

@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 October 13, 2020 15:36 Inactive
@dkent600 dkent600 temporarily deployed to alchemy-add3boxthreads-dpi9dn1 October 13, 2020 16:12 Inactive
@dkent600 dkent600 merged commit 8b10671 into dev-2 Oct 13, 2020
@orenyodfat orenyodfat deleted the add3BoxThreads branch November 2, 2020 14:47
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.

3 participants