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

feat(InlineLoading): add support for warning status #4810

Closed

Conversation

janhassel
Copy link
Member

This PR adds the option for a warning status to the InlineLoading component.

Changelog

New

  • warning status in InlineLoading

Changed

  • Changed current error icon to ErrorFilled icon to be consistent with the filled finished state and improve contrast for warning.

@janhassel janhassel requested a review from a team as a code owner December 3, 2019 15:00
@ghost ghost requested review from aledavila and dakahn December 3, 2019 15:01
@netlify
Copy link

netlify bot commented Dec 3, 2019

Deploy preview for carbon-elements ready!

Built with commit b09ce89

https://deploy-preview-4810--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Dec 3, 2019

Deploy preview for carbon-components-react ready!

Built with commit b09ce89

https://deploy-preview-4810--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Dec 3, 2019

Deploy preview for the-carbon-components ready!

Built with commit b09ce89

https://deploy-preview-4810--the-carbon-components.netlify.com

@asudoh asudoh requested a review from a team December 3, 2019 22:02
@ghost ghost requested review from aagonzales and removed request for a team December 3, 2019 22:02
@asudoh
Copy link
Contributor

asudoh commented Dec 3, 2019

New UX feature, requires design review.

@janhassel
Copy link
Member Author

@emyarod Oh yeah, right. I was wondering why it wasn't else-if and didn't think about the returns. Thank you!

@dakahn dakahn removed their request for review December 4, 2019 23:43
@joshblack
Copy link
Contributor

Hey @janhassel! Is there a corresponding issue we could look at for this feature?

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

Agreed, should be the filled version.

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @janhassel!

@janhassel
Copy link
Member Author

@joshblack We haven't opened an issue for this, but I can send you some information about the use case via Slack if you want.

@joshblack
Copy link
Contributor

@janhassel would you mind opening a ticket for it just so we can better understand the use-case/goal for adding it in?

Separately, I think the changes to the error icons and what-not are perfect so it might make sense to get that in with this PR and then do the net-new functionality in a later one once we understand the use-case.

@janhassel
Copy link
Member Author

@joshblack I opened an issue with a sample use case (committing in git): #4835

@joshblack
Copy link
Contributor

Thanks @janhassel!

@joshblack
Copy link
Contributor

Just a quick note, still waiting for another dev review and for the linked issue to be signed off by design 👍 After talking in our support channel it seems like @aagonzales might have only noticed the icon change (which is perfect 💯) and not the additional feature so will wait on them to verify in the linked issue!

@aagonzales
Copy link
Member

aagonzales commented Dec 10, 2019

@janhassel If the user is given the warning does that mean the load was not successful? The example is about merge conflicts but I'm trying to understand what is being recommended for the general guidance of when "warning" would be used instead of a binary success/fail.

@janhassel
Copy link
Member Author

@aagonzales Let's think about another example: a file uploader. We show the user the Inline Loading component while the file is being uploaded, once it's done, we switch to the finished state. If the upload failed, we show the error state.
But if a file with this name already exists on the destination, we can show the user a warning state, telling them that their file has been uploaded but they need to tell us whether or not to overwrite the existing file.

So to sum it up:
success / finished

  • everything worked, nothing to do

error

  • something didn't work out, the user can not proceed with the aspired action

warning

  • it worked, but due to an issue there is an extra step required for the user to accomplish their action
  • something didn't work out, but it is not crucial and the user can proceed. The action might be accomplished later on by the system.

Copy link
Contributor

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

Looks great on the dev side of things, just need @aagonzales to sign off and we can merge it in 👍

@joshblack
Copy link
Contributor

Any chance we could split this into just the icon change and then the proposal for the behavior? Would love to chat/learn more about the warning use-case and how we can incorporate into docs/guidance 👍

@janhassel
Copy link
Member Author

Definitely, I opened another PR just concerning the icon: #5018

@joshblack I'll send you a detailed example on Slack.

@asudoh
Copy link
Contributor

asudoh commented Jan 21, 2020

Any updates @janhassel @joshblack? Thanks!

@aagonzales
Copy link
Member

aagonzales commented Jan 21, 2020

Controversial statement: I think this feature is its own component and not an enhancement to inline loading. It's a status indictor and not solely about the state of loading. Jan explained the scenario to me further and I believe its a valid use-case I'm just not sure it belongs within the inline loading component.

image

@asudoh
Copy link
Contributor

asudoh commented Jan 24, 2020

@aagonzales Would you have a chance to other designer peers and see what we should do with this PR...? Thanks!

@joshblack
Copy link
Contributor

joshblack commented Feb 19, 2020

Closing this out in favor of the proposal over in: #4835

Thanks again for talking through this stuff @janhassel!

@joshblack joshblack closed this Feb 19, 2020
@janhassel janhassel deleted the inline-loading-warning branch July 21, 2020 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants