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

[Cookies] Increased cookies banner font size for med/large screens to improve readability #2611

Merged

Conversation

luisramos0
Copy link
Contributor

What? Why?

Closes #2597

Increased font size for med/large screens and adapted CSS to keep the text and button visible in all screen sizes.

What should we test?

The cookies banner should be tested in terms of UX in different screen sizes. Functionality was not changed.

Release notes

Changelog Category: Changed

Cookies banner font size increased.

@sigmundpetersen sigmundpetersen changed the title Imcreased cookies banner font size for med/large screens to improve readability nmcreased cookies banner font size for med/large screens to improve readability Aug 29, 2018
@sigmundpetersen sigmundpetersen changed the title nmcreased cookies banner font size for med/large screens to improve readability Increased cookies banner font size for med/large screens to improve readability Aug 29, 2018
@luisramos0 luisramos0 self-assigned this Aug 29, 2018
@luisramos0 luisramos0 force-pushed the cookies_banner_font_size branch from daffca5 to 53e7fcb Compare August 29, 2018 23:21
@luisramos0
Copy link
Contributor Author

I am not sure but I dont think the broken build is related to this PR...

the error is:
Using embedded shopfront functionality using iframes allows shopping and checkout
Failure/Error: expect(page).to have_text 'Your shopping cart'
expected to find text "Your shopping cart" in "Powered by Open Food Network English Castellano English Login 1 items My Embedded Hub Herndon Next order closing in 2 days Ready for ABOUT MY EMBEDDED HUB PRODUCERS CONTACT GROUPS Ruby on Rails Framed Apples from Enterprise 333 Framed Apples 1g, S $19.99 $19.99". (However, it was found 2 times including non-visible text.)
# ./spec/features/consumer/shopping/embedded_shopfronts_spec.rb:53:in `block (4 levels) in <top (required)>'

@mkllnk
Copy link
Member

mkllnk commented Aug 30, 2018

I just ran Semaphore again and the same spec failed again. I think it needs fixing even though it may be unrelated to this PR.

@luisramos0
Copy link
Contributor Author

I rerun the build and its green now.

@Matt-Yorkley
Copy link
Contributor

Matt-Yorkley commented Aug 31, 2018

I have a suspicion we might see this failing spec again, if the original spec was checking for something on the page which is now covered by the cookie banner once the page has fully loaded.

We can keep an eye on it, but maybe we can configure feature specs to run without showing the cookie banner by default, and only show the banner in the specific tests that need it?

@luisramos0
Copy link
Contributor Author

Yes Matt! I need to leave global config as it was before the test. Which is banner disabled. That is probably causing the error. I'll do that and add a commit to this PR. Also, Ill put that (must keep global state in your tests) to the code conventions.
Thanks!

@Matt-Yorkley
Copy link
Contributor

Great! I've seen other random failing feature specs this week for "missing" UI elements which then passed after a restart, so I'm guessing it's the same thing.

@Matt-Yorkley
Copy link
Contributor

P.S. @luisramos0 for debugging issues like this locally you can add the line save_and_open_screenshot temporarily to a spec at the point where the issue is, and it will give you a screenshot of what capybara is seeing.

@luisramos0
Copy link
Contributor Author

Thanks @Matt-Yorkley
I didnt know about that trick to get a screenshot, it's great! I added it here:
https://github.com/openfoodfoundation/openfoodnetwork/wiki/Testing-and-Rspec-Tips#save_and_open_screenshot
I have a little local script that switches to selenium :-)

The convention "leave config as it was before the test" was already here:
https://github.com/openfoodfoundation/openfoodnetwork/wiki/Testing-and-Rspec-Tips#clearing-app-config-changes-from-cache

I have added a new commit to fix it for the cookies_spec.

Thanks guys!


# keeps config unchanged
around do |example|
original_config_value = Spree::Config[:cookies_policy_matomo_section]
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you've added this for these 2 specs, but is there a global config to disable it on all the other specs by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean but the default value for it is false. so, you either get it nil or false in all other tests. I think that's enough.
Is there a place for global test config where I could set it to false? I can do that as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, I wasn't sure about the defaults.

@luisramos0 luisramos0 changed the title Increased cookies banner font size for med/large screens to improve readability [Cookies] Increased cookies banner font size for med/large screens to improve readability Sep 2, 2018
@luisramos0
Copy link
Contributor Author

I think there are no pending comments. Moving to Test Ready.

@luisramos0 luisramos0 added the pr-staged-fr staging.coopcircuits.fr label Sep 3, 2018
@luisramos0
Copy link
Contributor Author

I just staged this one here: https://staging.openfoodfrance.org/
I also went to the backoffice and activated the banner to have a look myself.

@myriamboure myriamboure self-assigned this Sep 3, 2018
@myriamboure
Copy link
Contributor

I used Mozilla "test views" developer tool, pretty useful to check how it looks like on various screen sizes ;-) It doesn't deserve testing note, just a screen share:
It is more readable and decent in term of appearance, so good to go.
capture du 2018-09-03 13-15-45
I checked also on mobile as well of course, all good.

@RachL RachL removed the pr-staged-fr staging.coopcircuits.fr label Sep 3, 2018
@mkllnk mkllnk merged commit 0d2fa3d into openfoodfoundation:master Sep 4, 2018
@luisramos0 luisramos0 deleted the cookies_banner_font_size branch September 4, 2018 09:47
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.

7 participants