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

tweak ephemeral error messages #21876

Merged
merged 10 commits into from Jan 8, 2020
Merged

tweak ephemeral error messages #21876

merged 10 commits into from Jan 8, 2020

Conversation

@joshblum
Copy link
Member

joshblum commented Jan 2, 2020

patch does the following:

  • applies supersedes to ERROR type messages so erred exploding messages can have proper explode now info.
  • hides error messages on already exploded messages but not ash lines
  • tweaks error message copy/display

image

cc @cecileboucheron let me know if you want any other copy tweaks on the error text.

@joshblum joshblum requested review from mmaxim and keybase/react-hackers Jan 2, 2020
@joshblum

This comment has been minimized.

Copy link
Member Author

joshblum commented Jan 2, 2020

(ci flakes unrelated)

Copy link
Contributor

songgao left a comment

Some nits; I only looked at react code.

@@ -511,7 +511,7 @@ protocol local {
MessageType messageType;
gregor1.Time ctime;
boolean isEphemeral;
boolean isEphemeralExpired;
union { null, string } explodedBy;

This comment has been minimized.

Copy link
@songgao

songgao Jan 2, 2020

Contributor

Can we have both? This way JS side don't need to do the comparison.

This comment has been minimized.

Copy link
@joshblum

joshblum Jan 6, 2020

Author Member

We'd have to have a derived type that computes the value before sending it to the frontend. To keep things simple I just let the frontent derive it, we do a similar calculation here https://github.com/keybase/client/pull/21876/files#diff-ff27bea5c579bc43b5b22d046db6acc5R1214

<Kb.Text type="BodySmall" style={this._isExploding() ? styles.failExploding : styles.fail}>
{this._isExploding() && (
<Kb.Icon fontSize={16} boxStyle={styles.failExplodingIcon} type="iconfont-block" />
)}{' '}

This comment has been minimized.

Copy link
@songgao

songgao Jan 2, 2020

Contributor

Does this insert a space when this._isExploding() === false?

This comment has been minimized.

Copy link
@joshblum

joshblum Jan 6, 2020

Author Member

used some padding instead!

@joshblum joshblum requested a review from songgao Jan 6, 2020
@songgao
songgao approved these changes Jan 6, 2020
Copy link
Contributor

songgao left a comment

I'm OK with this, but it seems space would be more consistent. Does this not work?

          {this._isExploding() && (
            <>
              <Kb.Icon fontSize={16} boxStyle={styles.failExplodingIcon} type="iconfont-block" />
              {' '}
            </>
          )}
@songgao

This comment has been minimized.

Copy link
Contributor

songgao commented Jan 7, 2020

I glanced over the Go side, and it LGTM too. But I'm not super familiar with this code, so feel free to wait for Mike.

@joshblum

This comment has been minimized.

Copy link
Member Author

joshblum commented Jan 7, 2020

I'm OK with this, but it seems space would be more consistent. Does this not work?

          {this._isExploding() && (
            <>
              <Kb.Icon fontSize={16} boxStyle={styles.failExplodingIcon} type="iconfont-block" />
              {' '}
            </>
          )}

Oh great, I wasn't sure the syntax, did not know about <>...</>, thanks!

@joshblum joshblum force-pushed the joshblum/ek-HOTPOT-1531 branch from 10c7911 to 6ef0cab Jan 8, 2020
go/chat/supersedes.go Outdated Show resolved Hide resolved
@joshblum joshblum force-pushed the joshblum/ek-HOTPOT-1531 branch from 6ef0cab to 2289c30 Jan 8, 2020
@joshblum joshblum requested a review from mmaxim Jan 8, 2020
@mmaxim
mmaxim approved these changes Jan 8, 2020
@joshblum joshblum merged commit c02529e into master Jan 8, 2020
0 of 2 checks passed
0 of 2 checks passed
ci/circleci CircleCI is running your tests
Details
continuous-integration/jenkins/pr-head This commit is being built
Details
@joshblum joshblum deleted the joshblum/ek-HOTPOT-1531 branch Jan 8, 2020
jakob223 added a commit that referenced this pull request Jan 14, 2020
* wip

* Hide exploded messages that have errors

* ui

* block icon

* fix test

* padding

* don't hide ash on errors

* fix typo

* use space instead

* name return vars
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.