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

Remove all growls on NewDot #13229

Closed
conorpendergrast opened this issue Dec 1, 2022 · 42 comments
Closed

Remove all growls on NewDot #13229

conorpendergrast opened this issue Dec 1, 2022 · 42 comments
Labels
Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 Not a priority

Comments

@conorpendergrast
Copy link
Contributor

conorpendergrast commented Dec 1, 2022

On hold for WAQ focus

As identified in this issue and discussed in this thread, we have some remaining growls that are now out of style. This is not a bug, but still needs to be fixed. @luacmartins found the following that still have growls:

  1. Interacting with the checkbox in Workspace > Manage member page
  2. Adding/Deleting PaypalMe address
  3. Failure updating the User avatar
  4. Failure to create, update or delete a Workspace
  5. Error downloading a wallet statement

And we should remove them all.

@conorpendergrast conorpendergrast added Monthly KSv2 Internal Requires API changes or must be handled by Expensify staff labels Dec 1, 2022
@conorpendergrast conorpendergrast self-assigned this Dec 1, 2022
@trjExpensify
Copy link
Contributor

Opening external links when offline

I believe these growls were intentionally left during the Offline API refactors project. The notion being to :do-nothing: as the redirects will go away as we bring those features into NewDot as we progress with reunification. CC: @robertjchen

@conorpendergrast
Copy link
Contributor Author

Ok, removed from the list!

@kadiealexander
Copy link
Contributor

Assigning this to myself, as part of my mentorship this year is to be working towards fixing E/app issues. If this becomes an urgent priority please let me know.

@kadiealexander kadiealexander self-assigned this Dec 8, 2022
@JmillsExpensify
Copy link

Nice, I'm glad we have a project for this. For what it's worth, I would also remove this one from the list. It's ridiculously far out and something we fix post-reunification.

Error downloading a wallet statement

@melvin-bot melvin-bot bot added the Overdue label Jan 4, 2023
@kadiealexander
Copy link
Contributor

Not overdue, have been OOO from a family emergency. Going to dig into this over the next week.

@melvin-bot melvin-bot bot removed the Overdue label Jan 16, 2023
@JmillsExpensify
Copy link

Nice, this is a pretty good polish project that's worth picking up now. Happy to help with any questions.

@kadiealexander
Copy link
Contributor

Thanks Jason! I have a draft PR up with the first growl removed, going to plug away at the rest this week! Should I still hold off on "Error downloading a wallet statement"?

@JmillsExpensify
Copy link

Oh cool! That's awesome, I don't personally think removing that is an issue since the wallet statement is still a beta, though @joekaufmanexpensify can jump in and confirm one way or another.

@joekaufmanexpensify
Copy link
Contributor

Just for my own understanding, would we be replacing the growl with a different style of error message? Or just removing the growl and leave it failing silently for now?

I think either way is fine for now (since no one is really using the statement right now). Although if we aren't adding a new error now, I'll add this to the existing statements cleanup issue we have (which is on hold right now).

@yuwenmemon yuwenmemon changed the title [Hold for WAQ] Remove all growls Remove all growls Jan 19, 2023
@yuwenmemon
Copy link
Contributor

Removing the HOLD from this!

@JmillsExpensify
Copy link

@joekaufmanexpensify Yeah we'd replace the growl with a different message style.

@joekaufmanexpensify
Copy link
Contributor

Sounds good! Yeah if we're replacing the growl with a different error message style, that sounds great.

@joekaufmanexpensify
Copy link
Contributor

Just for my own understanding, have we decided what will be added in their place?

@luacmartins
Copy link
Contributor

IMO we should use the existing DotIndicatorMessage and Form error pattern when possible:

For example, here's a case where the modal was replaced with the DotIndicatorMessage component:

Screenshot 2023-01-25 at 10 34 27 AM

And here's how we show it in Form:

Screenshot 2023-01-25 at 10 34 41 AM

@JmillsExpensify
Copy link

Agree with @luacmartins

@joekaufmanexpensify
Copy link
Contributor

Makes sense, ty!

@kadiealexander
Copy link
Contributor

Is adding in the new indicator messages something I should do as a part of this issue, or will it be covered later? Just clarifying because the scope of the OP was very limited to removing growls.

@JmillsExpensify
Copy link

Sorry what's the new indicator messages?

@luacmartins
Copy link
Contributor

I think @kadiealexander is talking about the new error messages we'd put in place instead of the growl?

@JmillsExpensify
Copy link

Gotcha, hmm yeah I think we need the error messages to appear somewhere, if we are deprecating the growls where they appear today, right? Apologies if I'm missing something obvious.

@luacmartins
Copy link
Contributor

Yea, I agree

@kadiealexander
Copy link
Contributor

Okay! The OP is very limited to removing the current growls, thanks for the added context. I'm going to pause on actioning this while I figure out the new messaging for these. A few aren't errors, so I'm not sure if any sort of "success" message should be put in place instead.

Not an error, need to determine what message to show (if any):

  • Interacting with the checkbox in Workspace > Manage member page
  • Adding/Deleting PaypalMe address (not an error)

Growl errors to be replaced with DotIndicatorMessage:

  • Failure updating the User avatar
  • Failure to create, update or delete a Workspace
  • Error downloading a wallet statement

@conorpendergrast
Copy link
Contributor Author

Looks like this is ongoing still @kadiealexander, is that right? No rush at all on my end

@JmillsExpensify
Copy link

Agree that there is no rush, though is would still be good to do this in the intermediate term. I think we don't have many growls as it is.

@melvin-bot
Copy link

melvin-bot bot commented Jul 3, 2023

@conorpendergrast, @kadiealexander, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

@kadiealexander
Copy link
Contributor

I've discussed this with my mentor @puneetlath and we agreed that the addition of different indicators blew this out of my realm of expertise. I'm unassigning so that this is not blocked on me.

@kadiealexander kadiealexander removed their assignment Jul 4, 2023
@kadiealexander kadiealexander removed Reviewing Has a PR in review Not a priority labels Jul 4, 2023
@conorpendergrast
Copy link
Contributor Author

conorpendergrast commented Jul 4, 2023

@puneetlath @kadiealexander This still needs to be internal, correct?

@kadiealexander
Copy link
Contributor

Wrong Puneet! From everything I saw this could be external, but I could be wrong.

@conorpendergrast
Copy link
Contributor Author

Oh that's embarrassing, silly me

@puneetlath
Copy link
Contributor

Agreed, seems like it can definitely be external. If it's a little more involved, it could be a good one for an expert agency.

@conorpendergrast
Copy link
Contributor Author

@puneetlath Do you think you could update this with everything it needs to be external?

@melvin-bot melvin-bot bot added the Overdue label Sep 4, 2023
@melvin-bot melvin-bot bot removed the Overdue label Sep 5, 2023
@melvin-bot melvin-bot bot added the Overdue label Oct 9, 2023
@kadiealexander kadiealexander removed their assignment Oct 10, 2023
@melvin-bot melvin-bot bot removed the Overdue label Oct 10, 2023
Copy link

melvin-bot bot commented Dec 22, 2023

@conorpendergrast, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 Not a priority
Projects
None yet
Development

No branches or pull requests

8 participants