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

Generalize ErrorBanner Component #601

Closed
braughtg opened this issue Dec 21, 2022 · 15 comments · Fixed by #622
Closed

Generalize ErrorBanner Component #601

braughtg opened this issue Dec 21, 2022 · 15 comments · Fixed by #622
Assignees
Labels
enhancement Improvement to the project UI/UX User Interface / User Experience Improvement

Comments

@braughtg
Copy link
Member

There is an ErrorBannerComponent defined in the resources folder and demonstrated on the UI Subtab. There should also be a SuccessBannerComponent that can be used to display success messages (e.g. "Log has been saved." ect...).

One way to do this would be to generalize the ErrorBannerComponent so that a prop dictates if it is an ErrorBanner or a SuccessBanner. Another prop can determine how long the banner is displayed or if it must be clicked to be dismissed. This would help significantly with the consistency of the code and UI/UX across modules.

When this more general component is complete it should be fully integrated into all of the pages where it error and/or success banners are displayed. Currently these are:

  • Seeding Report
  • Transplanting Report
  • Seeding Input
  • Config
@braughtg braughtg added enhancement Improvement to the project UI/UX User Interface / User Experience Improvement labels Dec 21, 2022
@braughtg braughtg changed the title SuccessBanner Component Generalize ErrorBanner Component Jan 24, 2023
@braughtg
Copy link
Member Author

It would be ideal to generalize this component so that it can be added to the top of every page and be used for every message (error/success/etc.). Need

  • a design for this
  • prototype on the examples tab
  • integrate into all example tabs
  • integrate into seeding input and seeding report tabs

@FutzMonitor
Copy link
Collaborator

I will tackle this issue.

@FutzMonitor
Copy link
Collaborator

Potential Design

At the moment, the ErrorBannerComponent displays a red banner indicating an error has occurred. I would rename this component to the BannerComponent to reflect its eventual generalized purpose of displaying any kind of pop-up message (success/error/etc.).

The component makes use of a bootstrap element [link] that takes a class to define the color of the banner. It would be best if this class was bound to a prop in order to assign it the necessary message color depending on the error. We could assign it some kind of numerical number, like we discussed in Tuesday's meeting, and depending on the number the correct color is shown. Each parent page would have to keep track of certain things that could trigger the BannerComponent and change this value to make the color appropriate. It doesn't necessarily have to make the banner visible but it'd set the banner to the appropriate color.

The message is currently a prop passed into the component. Similar to the above proposal, perhaps a number can be passed into the prob which changes it to the appropriate message which would already be saved into the component. That way both props behave consistently.

Thoughts on these potential changes?

@braughtg
Copy link
Member Author

braughtg commented Jan 26, 2023

We could assign it some kind of numerical number, like we discussed in Tuesday's meeting, and depending on the number the correct color is shown.

Would it make sense to have the page use the appropriate Bootstrap value as the prop instead? That might be more flexible and also keep color use consistent across the site. Alternatively, maybe the banner supports just a few colors (error, success, message) and those are set by a prop using a string to keep them readable?

Each parent page would have to keep track of certain things that could trigger the BannerComponent and change this value to make the color appropriate. It doesn't necessarily have to make the banner visible but it'd set the banner to the appropriate color.

I would imagine that when an event occurs in the page it would set the color and displaying the banner - making it visible via a v-if or v-show within the page. Is there a use case you were thinking of where the page would set the banner color without making it visible?

The message is currently a prop passed into the component. Similar to the above proposal, perhaps a number can be passed into the prob which changes it to the appropriate message which would already be saved into the component.

I like the idea of the message being passed into component by a prop rather than being stored in the prop and set by a numeric value. To ensure consistency of messages we could put the standard messages into an object in resources similar to the way that was done with some of the common regular expressions.

@FutzMonitor
Copy link
Collaborator

Would it make sense to have the page use the appropriate Bootstrap value as the prop instead? That might be more flexible and also keep color use consistent across the site. Alternatively, maybe the banner supports just a few colors (error, success, message) and those are set by a prop using a string to keep them readable?

I thought this as well, allowing the prop to simply be the color needed for the banner, but I thought this numerical assignment would be better and store the colors within the component itself to have consistent coloring between all the pages.

I would imagine that when an event occurs in the page it would set the color and displaying the banner - making it visible via a v-if or v-show within the page. Is there a use case you were thinking of where the page would set the banner color without making it visible?

I was thinking of preparing for cases like these (where the presets for the component need to be set but the component itself doesn't need to be visible). I can't think of any specific scenarios at the moment, so it might be better for the time being to advance without this addressing these hypothetical cases.

I like the idea of the message being passed into component by a prop rather than being stored in the prop and set by a numeric value. To ensure consistency of messages we could put the standard messages into an object in resources similar to the way that was done with some of the common regular expressions.

Similar with the color of the banner, I was thinking of storing an array of these messages within the component itself and a numerical value being the deciding factor of which message is displayed. This would ensure consistency between all pages using the component. However, using the resources page achieves the same functionality. I'm thinking though what are the chances that we want each separate parent page to simply define and feed an array of object (with color and message) since maybe each custom message for each page might be unique? It could grant us greater flexibility perhaps but then each page would have to define this array of objects. What are your thoughts?

@braughtg
Copy link
Member Author

Would it make sense to have the page use the appropriate Bootstrap value as the prop instead? That might be more flexible and also keep color use consistent across the site. Alternatively, maybe the banner supports just a few colors (error, success, message) and those are set by a prop using a string to keep them readable?

I thought this as well, allowing the prop to simply be the color needed for the banner, but I thought this numerical assignment would be better and store the colors within the component itself to have consistent coloring between all the pages.

Good point! I propose that we define 3 specific string values (rather than integers) for readability of the code. Maybe error, success, message - though we can discuss the specific values. The component would internally define the colors and style (icon, etc) for those three types. That will generate the consistency across pages as you note.

@braughtg
Copy link
Member Author

Each parent page would have to keep track of certain things that could trigger the BannerComponent and change this value to make the color appropriate. It doesn't necessarily have to make the banner visible but it'd set the banner to the appropriate color.

I would imagine that when an event occurs in the page it would set the color and displaying the banner - making it visible via a v-if or v-show within the page. Is there a use case you were thinking of where the page would set the banner color without making it visible?

I was thinking of preparing for cases like these (where the presets for the component need to be set but the component itself doesn't need to be visible). I can't think of any specific scenarios at the moment, so it might be better for the time being to advance without this addressing these hypothetical cases.

I agree. But as I think about it more the page will be able to change the props being passed to the component at any time. This would be independent of the variable that is controlling the visibility of the element. I think that covers the behavior you were originally describing. So really, we should get that for free.

@braughtg
Copy link
Member Author

I like the idea of the message being passed into component by a prop rather than being stored in the prop and set by a numeric value. To ensure consistency of messages we could put the standard messages into an object in resources similar to the way that was done with some of the common regular expressions.

Similar with the color of the banner, I was thinking of storing an array of these messages within the component itself and a numerical value being the deciding factor of which message is displayed. This would ensure consistency between all pages using the component. However, using the resources page achieves the same functionality. I'm thinking though what are the chances that we want each separate parent page to simply define and feed an array of object (with color and message) since maybe each custom message for each page might be unique? It could grant us greater flexibility perhaps but then each page would have to define this array of objects. What are your thoughts?

The question of color / visual consistency and how to manage that has been addressed above. As far as messages go, I think using an object in resources makes sense. I imagine we will want things like "Seeding log has been saved" and "Transplanting log has been saved" for different pages. So each page could do this, but then there is a good chance of those messages being more different. If they are all in one place then when adding a new message, the others will be right there for comparison. Hopefully that will make them more likely to be consistent across pages. Thoughts?

@FutzMonitor
Copy link
Collaborator

The question of color / visual consistency and how to manage that has been addressed above. As far as messages go, I think using an object in resources makes sense. I imagine we will want things like "Seeding log has been saved" and "Transplanting log has been saved" for different pages. So each page could do this, but then there is a good chance of those messages being more different. If they are all in one place then when adding a new message, the others will be right there for comparison. Hopefully that will make them more likely to be consistent across pages. Thoughts?

So you're proposing that in resources we have this array or object of strings and whenever we need a to add a new message we simply add it to this data in the resources so that it's accessible to all pages? It wouldn't be a bad idea because I cannot imagine we'll be adding many message to it often. Sub-tabs like the Seeding Report and Transplanting Report would behave similarly that no new message would need to be added. New messages may need to be added for new sub-tabs (transplanting form, harvest form) but not enough I believe to discourage us from simply putting them all in one data field.

Do I have it right?

@braughtg
Copy link
Member Author

So you're proposing that in resources we have this array or object of strings and whenever we need a to add a new message we simply add it to this data in the resources so that it's accessible to all pages?

Yes. This is right. So it would be much like the regexMap object that holds the commonly used regular expressions.

Sub-tabs like the Seeding Report and Transplanting Report would behave similarly that no new message would need to be added.

I can still imagine that we might want the messages to be customized to the page so that they use "Seeding Report" or "Transplanting Report" in them. For example, when saving an edit it might say "Seeding log successfully updated" or "Error saving Seeding log".

I could imagine the object containing these messages having a collection of messages for each page: SeedingInputSaveError, SeedingInputSaveSuccess, SeedingInputCanceled, etc...

The advantage of putting them all in one place is that it will be easy to pattern match across pages when new messages are added. This should help ensure consistency.

@FutzMonitor
Copy link
Collaborator

Yes. This is right. So it would be much like the regexMap object that holds the commonly used regular expressions.

I understand. I will follow this design during the generalization of the component.

I can still imagine that we might want the messages to be customized to the page so that they use "Seeding Report" or "Transplanting Report" in them. For example, when saving an edit it might say "Seeding log successfully updated" or "Error saving Seeding log".

I could imagine the object containing these messages having a collection of messages for each page: SeedingInputSaveError, SeedingInputSaveSuccess, SeedingInputCanceled, etc...

The advantage of putting them all in one place is that it will be easy to pattern match across pages when new messages are added. This should help ensure consistency.

That would definitely help with consistency.

Well I think I have a good idea of where to start with the generalization of the ErrorBannerComponent. I'll work on it some this weekend and hopefully have something operational to present soon enough.

@braughtg
Copy link
Member Author

I'll work on it some this weekend and hopefully have something operational to present soon enough.

Sounds good. I would suggest starting by writing the JSDoc for the component and the resources object so that they are more precisely defined before beginning with any code. That might help us to uncover any additional important details that we may have missed to this point.

@FutzMonitor
Copy link
Collaborator

Yes. This is right. So it would be much like the regexMap object that holds the commonly used regular expressions.

Currently the regexMap data is located in the RegularExpressions.js file. Should I create a new file to house these messages or should we generalize the RegularExpressions.js file to hold miscellaneous data like these messages?

What are the benefits of keeping these in separate files and the benefits of keeping them all in one file?

@braughtg
Copy link
Member Author

Should I create a new file to house these messages or should we generalize the RegularExpressions.js file to hold miscellaneous data like these messages?

Yes, we should use a separate file for the messages.

What are the benefits of keeping these in separate files and the benefits of keeping them all in one file?

The messages and the regular expressions have very different purposes. So having them in separate files with appropriate names will help keep the repository structure more intuitive. Seeing files named RegularExpressions.js and BannerMessages.js communicate something about their contents that a combined file likely would not. This is will enhancing readability when these files are imported and also when someone is going looking for the regular expressions or the messages in the repo.

@braughtg
Copy link
Member Author

Some additional thoughts on the design of the BannerComponent:

  • It should (if possible) be sticky. That is it should appear at the top of the visible viewport, even if the page is not scrolled to the top. This will be nice if an error occurs when the user is scrolled down to the bottom of a page. When not visible, the banner should not consume any screen real estate.
  • The banner should allow for use
    • With a timeout - so that the banner goes away n seconds after it is shown.
    • With an X to dismiss it - so that the banner remains in place until the user clicks the X.

braughtg pushed a commit that referenced this issue Apr 4, 2023
__Pull Request Description__
This PR closes #601. This pull request generalizes the functionality of
the `ErrorBannerComponent` into the newer `BannerComponent` which can
display error, success, and generic message banners. Additionally, the
refactored component can support timeout (instead of clicking an _X_ to
close the component, it'll collapse itself after 5secs) and is has a
sticky style which makes it appear on screen without the intrusive auto
scroll.

This PR will not remove the pre-existing `ErrorBannerComponent` from the
project. That is something that will need to be done in a separate PR
that also replaces those instances with the new `BannerComponent`.

This PR is **NOT** ready to be merged into main. 
Prior to merging, the following items must be completed. 
- [x] Complete code review 
- [x] Remove all instances of the `ErrorBannerComponent` from the
ui.html page.
- [x] Update the end-2-end test for the ui.html page. 
- [x] Remove all lingering debugging code from the PR. This includes
unnecessary comments.
- [x] ~~Regenerate documentation~~ (This is generated locally now)


Edit: Closes #635. 

---
__Licensing Certification__

FarmData2 is a [Free Cultural
Work](https://freedomdefined.org/Definition) and all accepted
contributions are licensed as described in the LICENSE.md file. This
requires that the contributor holds the rights to do so. By submitting
this pull request __I certify that I satisfy the terms of the [Developer
Certificate of Origin](https://developercertificate.org/)__ for its
contents.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to the project UI/UX User Interface / User Experience Improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants