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

Fixes #4242 so that css file doesn't make the warning message look scary #4462

Conversation

AweysAhmed
Copy link
Contributor

@AweysAhmed AweysAhmed commented Jun 20, 2024

Resolves #4242

Description

This PR updates the CSS so that they warning text appears in plaintext and not in red.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I tested this changes locally to ensure removing the text-colour changes the warning message to black or close to black.

In order to test this out you will need to

  • set up a new distribution (Distributions | New Distribution) , a new donation (Donation |New Donation), and a new purchase (Purchases | New Purchase) that are all using a particular item (e.g. Adult Briefs L/XL)

  • then go into Inventory | Inventory Adjustments

  • Enter new Inventory Adjustments to take the levels for that item for each storage location down to 0

  • Go to Inventory|Items & Inventory

  • Deactivate the item

  • Go to Distributions

  • you should see a "Has Inactive Items" message. The style of this has has be changed

  • click View on that item, and scroll to the bottom

  • you should see a message indicating why you can't correct this. The style of this message has be changed

  • Go to Donations

  • Click view on the donation you entered and scroll to the bottom

  • you should see a message indicating why you can't correct this. The style of this message has be changed

  • Go to Purchases

  • Click view on the purchase you entered and scroll to the bottom

  • you should see a message indicating why you can't correct this. The style of this message has be changed

  • Before

    Screenshot 2024-06-19 at 1 12 38 PM Screenshot 2024-06-23 at 4 11 42 PM Screenshot 2024-06-23 at 4 15 24 PM Screenshot 2024-07-02 at 12 45 52 PM

    After

    Screenshot 2024-06-19 at 10 01 38 PM

    Screenshot 2024-06-23 at 4 13 55 PM Screenshot 2024-06-23 at 4 15 37 PM Screenshot 2024-06-23 at 4 15 37 PM

@AweysAhmed AweysAhmed marked this pull request as ready for review June 23, 2024 20:27
@AweysAhmed AweysAhmed changed the title edited css file to make message less omnious Fixes [#4242] so that css file doesn't make the warning message look scary Jun 23, 2024
@AweysAhmed AweysAhmed changed the title Fixes [#4242] so that css file doesn't make the warning message look scary Fixes #4242 so that css file doesn't make the warning message look scary Jun 23, 2024
@cielf
Copy link
Collaborator

cielf commented Jun 23, 2024

@AweysAhmed I'll take a better look at this tomorrow. My initial thoughts are 'but I think there are other warnings, that might need more than plain text', and 'maybe we need a different name for this to indicate that it's a low-intensity warning'.

@AweysAhmed
Copy link
Contributor Author

@AweysAhmed I'll take a better look at this tomorrow. My initial thoughts are 'but I think there are other warnings, that might need more than plain text', and 'maybe we need a different name for this to indicate that it's a low-intensity warning'.

That is a good point. I was thinking of changing the class CSS class name to div.low_priority_warning and that way we can create another class for medium and hight priority warnings.

@cielf
Copy link
Collaborator

cielf commented Jun 24, 2024

@AweysAhmed I'll take a better look at this tomorrow. My initial thoughts are 'but I think there are other warnings, that might need more than plain text', and 'maybe we need a different name for this to indicate that it's a low-intensity warning'.

That is a good point. I was thinking of changing the class CSS class name to div.low_priority_warning and that way we can create another class for medium and hight priority warnings.

May i suggest that, instead, you create a new class for div.low_priority_warning, and leave div.warning as it was. That way, if there is anything else that is using the warning class, it won't be affected.

@AweysAhmed
Copy link
Contributor Author

@AweysAhmed I'll take a better look at this tomorrow. My initial thoughts are 'but I think there are other warnings, that might need more than plain text', and 'maybe we need a different name for this to indicate that it's a low-intensity warning'.

That is a good point. I was thinking of changing the class CSS class name to div.low_priority_warning and that way we can create another class for medium and hight priority warnings.

May i suggest that, instead, you create a new class for low_priority_warning, and leave warning as it was. That way, if there is anything else that is using the warning class, it won't be affected.

Thank you. That makes sense. I'll make that change.

@AweysAhmed
Copy link
Contributor Author

@AweysAhmed I'll take a better look at this tomorrow. My initial thoughts are 'but I think there are other warnings, that might need more than plain text', and 'maybe we need a different name for this to indicate that it's a low-intensity warning'.

That is a good point. I was thinking of changing the class CSS class name to div.low_priority_warning and that way we can create another class for medium and hight priority warnings.

May i suggest that, instead, you create a new class for low_priority_warning, and leave warning as it was. That way, if there is anything else that is using the warning class, it won't be affected.

Thank you. That makes sense. I'll make that change.

I forgot to mention that the current div.warning CSS class is only used in the three locations that display the warning messages that need to be updated. So my changes will not impact any other erb files.

So maybe we should create the div.low_priority_warning and if in the future we need a more urgent warning message we can create it.

@cielf
Copy link
Collaborator

cielf commented Jun 24, 2024

Sure -- if we're sure it's not used elsewhere. I know there are other warning-type messages, but they may be using a different class.

@AweysAhmed
Copy link
Contributor Author

Sure -- if we're sure it's not used elsewhere. I know there are other warning-type messages, but they may be using a different class.

@cielf @dorner I have added a missing screenshot and double-checked to make sure the CSS class .warning isn't used anywhere else.

Since we are only using it the erb files that I have changed I have renamed the CSS class to .low_priority_warning and remove the red colour so that if we need a more urgent warning CSS class we can create a .warning class.

Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

LGTM

@cielf cielf merged commit f0dfb02 into rubyforgood:main Jul 3, 2024
19 checks passed
Copy link
Contributor

github-actions bot commented Jul 7, 2024

@AweysAhmed: Your PR Fixes #4242 so that css file doesn't make the warning message look scary is part of today's Human Essentials production release: 2024.07.07.
Thank you very much for your contribution!

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.

Change style of messages regarding inactive items
2 participants