Implements our StyleGuide #7284

jibees · 2021-03-30T14:04:29Z

What? Why?

This PR is here to create a StyleGuide inside OFN using view_component and storybook

How it works?

Components should now be created with https://viewcomponent.org with helper method: rails generate component Example title --sidecar (see document for more details)
Stories should be put into the spec/components/stories folder and then transformed to be ready for Storybook with the command: rake view_component_storybook:write_stories_json. This creates a example_component.stories.json, which is gitignored, but readable by StoryBook.
Once this is done, we have to start the rails server, and then StoryBook itself with the following command: yarn storybook
Enjoy!

Source:

Release notes

Create our styleguide

Changelog Category: Technical changes

What should we test?

Test the Print to PDF functionality everywhere in the app.
Test CSS and fonts look okay
Check the enterprise title renders okay in shopfronts

Issues I faced:

MonkeyPatch

For now, this is not functional. Indeed, view_component is using a monkeypatch to override #render method since we're using Rails5.0 (seems to be ok with Rails5.2, but not sure): https://github.com/github/view_component/blob/main/lib/view_component/render_monkey_patch.rb
As WickedPdf used the same monkeypatch, seems to be a conflict, as noticed here: ViewComponent/view_component#288
Not sure to totally understand, still need some investigation.
For the record, the error in the rails console:

....
wicked_pdf (2.1.0) lib/wicked_pdf/pdf_helper.rb:46:in `render_with_wicked_pdf'
wicked_pdf (2.1.0) lib/wicked_pdf/pdf_helper.rb:30:in `render'
wicked_pdf (2.1.0) lib/wicked_pdf/pdf_helper.rb:46:in `call'
wicked_pdf (2.1.0) lib/wicked_pdf/pdf_helper.rb:46:in `render_with_wicked_pdf'
wicked_pdf (2.1.0) lib/wicked_pdf/pdf_helper.rb:30:in `render'
wicked_pdf (2.1.0) lib/wicked_pdf/pdf_helper.rb:46:in `call'
....

Resolved by cherrypicked: 2b19ea9 and 3893c07

luisramos0 · 2021-03-30T22:11:39Z

This is nice 👏
I wonder why so many changes in yarn.lock.

Could you share a screenshot of storybook running?

jibees · 2021-03-31T07:24:54Z

I wonder why so many changes in yarn.lock.

Storybook has a lot of dependencies, but yes, yarn.lock has large modification. Maybe it has also update some of our pre-existing dependencies?

Could you share a screenshot of storybook running?
Sure, very simple at this time:

NB: the component itself is not displaying because of the monkeypatch conflict I guess.

luisramos0 · 2021-03-31T07:43:29Z

Awesome! Well done 👏

…

On Wed, 31 Mar 2021, 08:25 jibees, ***@***.***> wrote: I wonder why so many changes in yarn.lock. Storybook has a lot of dependencies, but yes, yarn.lock has large modification. Maybe it has also update some of our pre-existing dependencies? Could you share a screenshot of storybook running? Sure, very simple at this time: [image: Capture d’écran 2021-03-31 à 09 21 17] <https://user-images.githubusercontent.com/296452/113106255-d8cf8280-9202-11eb-924f-e81b64fc882e.png> *NB*: the component itself is not displaying because of the monkeypatch conflict I guess. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#7284 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMQPOSVZJOPAQTANJG6DNDTGLE5NANCNFSM42CEAL2A> .

Matt-Yorkley · 2021-03-31T16:28:28Z

Okay, option 1 is to use the ViewComponents config option render_monkey_patch_enabled = false, and then render components in the app using the #render_component method. This should work fine, but I think it won't work with StoryBook for now.

Option 2 might get a bit messy, we'll have to add an additional monkey-patch to wicked_pdf ourselves. We might already have a problem there, in that the current version of wicked_pdf uses #alias_method_chain, which is removed in Rails 5.2... there's definitely some complexity there. It might look like this.

jibees · 2021-03-31T19:08:52Z

Thanks a lot @Matt-Yorkley: I've cherrypicked your two commits, and now the error disappeared. 💪 🙏

Now I'm facing over CORS errors:

iframe.html?id=example-component--with-short-text&viewMode=story:1 Access to fetch at 'http://localhost:3000/rails/stories/example_component/with_short_text?title=OK' from origin 'http://localhost:61898' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.

jibees · 2021-03-31T19:28:11Z

There you go!

Matt-Yorkley · 2021-03-31T19:31:22Z

I've cherrypicked your two commits, and now the error disappeared

It's possible those two commits might break PDF creation elsewhere in the app, it was a quick attempt at a potential fix, but it would need careful testing...

jibees · 2021-04-01T12:47:36Z

I've cherrypicked your two commits, and now the error disappeared

It's possible those two commit might break PDFs elsewhere in the app, it was a quick fix, but it would need careful testing...

I already tried to make print to pdf functionality to work on my machine, but i always failed... ;(

codecov · 2021-04-01T12:54:16Z

Codecov Report

Merging #7284 (9a670c7) into master (6dc2380) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #7284   +/-   ##
=======================================
  Coverage   93.12%   93.12%           
=======================================
  Files         631      633    +2     
  Lines       18090    18096    +6     
=======================================
+ Hits        16846    16852    +6     
  Misses       1244     1244

Impacted Files	Coverage Δ
app/components/distributor_title_component.rb	`100.00% <100.00%> (ø)`
app/components/example_component.rb	`100.00% <100.00%> (ø)`
app/controllers/spree/admin/orders_controller.rb	`83.33% <100.00%> (ø)`
app/services/invoice_renderer.rb	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6dc2380...9a670c7. Read the comment docs.

Matt-Yorkley · 2021-04-02T12:15:30Z

Hot tip: spec/services/invoice_renderer_spec.rb needs a little tweak on line 38, like this:

--  .to receive(:render_to_string)
++  .to receive(:render_to_string_with_wicked_pdf)

jibees · 2021-04-02T14:11:02Z

Hot tip: spec/services/invoice_renderer_spec.rb needs a little tweak on line 38, like this:
--  .to receive(:render_to_string)
++  .to receive(:render_to_string_with_wicked_pdf)

Thanks. I squashed your commit. 🙏

package.json

Gemfile

app/controllers/stories_controller_decorator.rb

mkllnk

Great work! I really appreciate how you take every challenge we through at you and learn along the way. You are making excellent progress.

I added some questions for my understanding. And do you think it would be possible to create a real example that we can use? Even if it's really small, like a text box somewhere.

Gemfile

app/components/example_component.rb

.gitignore

config/application.rb

config/initializers/wicked_pdf.rb

app/controllers/stories_controller_decorator.rb

Matt-Yorkley · 2021-04-22T10:59:17Z

Okay, app/controllers/spree/admin/orders_controller.rb line 92 needs this change:

-- render InvoiceRenderer.new.args(@order)
++ render_with_wicked_pdf InvoiceRenderer.new.args(@order)

and this also indicates we have inadequate test coverage on that specific bit of PDF invoice printing, right? We can make a separate issue to cover it 👍

Are there any cases where we send PDFs attached to emails?

The template for this email ☝️ basically just says "...please find attached invoice..."

Edit: yep, it's in the same controller (line 81) but it goes via InvoiceRenderer#render_to_string, which we've already updated 👍

filipefurtad0 · 2021-04-22T11:48:00Z

Thanks for feedback @Matt-Yorkley ! 👍
Created two tech-debt issues to cover:

the missing test case on pdf/invoice creation - Add coverage on pdf creation - Order edit > Actions > Print Invoice #7455
email with attached pdf/invoice - [Release Test Automation] Add coverage on email w/ attached pdf - Order edit > Actions > Send Invoice #7456

jibees · 2021-04-22T12:31:31Z

@sauloperez @Matt-Yorkley
Draft: https://github.com/openfoodfoundation/openfoodnetwork/wiki/Creating-new-components Feel free to comment/edit/whatever.
Thanks a lot guys!

jibees · 2021-04-22T12:33:33Z

Thanks a lot for your tests, and for open issues.
I can do a test, but unfortunately, I have never been able to print to PDF on my machine. I still have a problem around wicked_pdf ... @Matt-Yorkley can you please take a look at these two problems?

@filipefurtad0 I've added dependencies for this PR : blocked by #7456 and #7455

Matt-Yorkley · 2021-04-22T12:46:43Z

can you please take a look at these two problems?

Yeah, I can take those two issues 👍

I have never been able to print to PDF on my machine.

Me neither, but I think we recently merged a change to master that means we can now run those PDF tests locally. I haven't re-checked it yet though.

As long as it passes manual testing here, I think we can cover those test coverage issues separately, without blocking this PR 👌

jibees · 2021-04-22T13:07:14Z

As long as it passes manual testing here, I think we can cover those test coverage issues separately, without blocking this PR 👌

Yes, you're right, I agree. I remove dependencies.

- Documentation : https://viewcomponent.org/ - Our template engine generator is haml

- With command: `rails generate component Example title --sidecar` - Delete generated spec file

- Add `ViewComponent::TestHelpers` to get the helper method `render_inline` - Use Capybara to expect some assertion

- Via `view_component_storybook` : https://github.com/jonspalmer/view_component_storybook - Configure both `view_component_storybook` and `storybook` - Add two addons: `@storybook/addon-docs` and `@storybook/addon-controls` Update config comment for clarity

- Thus, component can inherit from all CSS rules, rendering is as close as to the truth - Still need to import `admin` css file if we want to create components for admin part (and we probably want too). This will probably lead to overlapping problems between all CSS rules... - This is a bit hacky, but as our CSS files are packed by rails I didn't find any better solution. We could also use any sass loader, but with this solution we can be confident on the fact that the generated CSS is the one used by the server that finally displays the component

- Storybook need to access through a GET method on `/rails/stories/**` served by `ViewComponent::Storybook::StoriesController` - Configure exact policy with an initialized filter

- Once story has been created, still need to generate the story itself (in json file format). Help with command: `rake view_component_storybook:write_stories_json`

- Simple wrapper around `<h3 />` for distributor title, used in the shop front

and call WickedPdf::PdfHelper explicitly

jibees · 2021-04-22T13:19:36Z

@Matt-Yorkley
Modification done: 9a670c7 Thanks a lot for this one! 🙏
Moving back to Code Review

filipefurtad0 · 2021-04-22T14:51:00Z

Alright!

After your changes, clicking "Print Invoice" on the Actions menu or visiting /admin/orders/<order_nr>/print now prints the respective pdf invoice, for both invoice types.

Can't think of anywhere else where pdfs are created, I think we're good to go now.

Thanks for the quick fix 🚀

Matt-Yorkley · 2021-04-22T16:14:27Z

Alright, lets slap the button.

Thanks @jibees, sorry it was such a convoluted journey! 😅

jibees self-assigned this Mar 30, 2021

jibees force-pushed the styleguide branch from c8b6d92 to edeed24 Compare March 30, 2021 14:29

jibees force-pushed the styleguide branch from edeed24 to 3893c07 Compare March 31, 2021 19:06

jibees force-pushed the styleguide branch from 3893c07 to f93a9ba Compare March 31, 2021 19:27

jibees force-pushed the styleguide branch from f93a9ba to d0acd7a Compare March 31, 2021 19:30

jibees force-pushed the styleguide branch from d0acd7a to 7d84433 Compare April 1, 2021 12:42

jibees mentioned this pull request Apr 2, 2021

Backoffice Product List table UI uplift #7198

Open

jibees force-pushed the styleguide branch from 7d84433 to df3787b Compare April 2, 2021 14:09

Matt-Yorkley reviewed Apr 2, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

Matt-Yorkley reviewed Apr 2, 2021

View reviewed changes

Gemfile Outdated Show resolved Hide resolved

jibees force-pushed the styleguide branch 4 times, most recently from e696ce2 to 064bc70 Compare April 8, 2021 12:30

jibees commented Apr 8, 2021

View reviewed changes

app/controllers/stories_controller_decorator.rb Outdated Show resolved Hide resolved

jibees force-pushed the styleguide branch from 064bc70 to 6f0d321 Compare April 8, 2021 13:29

mkllnk reviewed Apr 9, 2021

View reviewed changes

jibees force-pushed the styleguide branch 2 times, most recently from 1c01eb4 to 025ac73 Compare April 9, 2021 09:10

This was referenced Apr 22, 2021

Add coverage on pdf creation - Order edit > Actions > Print Invoice #7455

Open

[Release Test Automation] Add coverage on email w/ attached pdf - Order edit > Actions > Send Invoice #7456

Open

jibees and others added 12 commits April 22, 2021 15:08

Add view_components to manage our components

e25cb1b

- Documentation : https://viewcomponent.org/ - Our template engine generator is haml

Create very first component

6c37461

- With command: `rails generate component Example title --sidecar` - Delete generated spec file

Add (very simple) spec for a ViewComponent component

390c8a8

- Add `ViewComponent::TestHelpers` to get the helper method `render_inline` - Use Capybara to expect some assertion

Add storybook

30bd932

- Via `view_component_storybook` : https://github.com/jonspalmer/view_component_storybook - Configure both `view_component_storybook` and `storybook` - Add two addons: `@storybook/addon-docs` and `@storybook/addon-controls` Update config comment for clarity

Configure CORS settings for StoriesController

453328a

- Storybook need to access through a GET method on `/rails/stories/**` served by `ViewComponent::Storybook::StoriesController` - Configure exact policy with an initialized filter

Remove WickedPdf monkey-patch on ActionController#render

1888118

Call WickedPdf::PdfHelper#render_to_string_with_wicked_pdf explicitly

57e5214

Create first story for ExampleComponent

39683bd

- Once story has been created, still need to generate the story itself (in json file format). Help with command: `rake view_component_storybook:write_stories_json`

Add new (and first) component: DistributorTitle

4781d6c

- Simple wrapper around `<h3 />` for distributor title, used in the shop front

Use the first view component DistributorTitleComponent for a shopfront

de592de

Print order needs render_with_wicked_pdf

9a670c7

and call WickedPdf::PdfHelper explicitly

jibees force-pushed the styleguide branch from 5d5a471 to 9a670c7 Compare April 22, 2021 13:18

Matt-Yorkley approved these changes Apr 22, 2021

View reviewed changes

filipefurtad0 added the pr-staged-au staging.openfoodnetwork.org.au label Apr 22, 2021

filipefurtad0 removed the pr-staged-au staging.openfoodnetwork.org.au label Apr 22, 2021

Matt-Yorkley merged commit 123e96e into openfoodfoundation:master Apr 22, 2021

jibees mentioned this pull request May 3, 2021

Create a style guide or pattern library for OFN styles openfoodfoundation/inception-pipe#34

Closed

jibees mentioned this pull request Dec 7, 2022

Remove @storybook #10113

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements our StyleGuide #7284

Implements our StyleGuide #7284

jibees commented Mar 30, 2021 •

edited

luisramos0 commented Mar 30, 2021

jibees commented Mar 31, 2021

luisramos0 commented Mar 31, 2021 via email

Matt-Yorkley commented Mar 31, 2021 •

edited

jibees commented Mar 31, 2021 •

edited

jibees commented Mar 31, 2021 •

edited

Matt-Yorkley commented Mar 31, 2021 •

edited

jibees commented Apr 1, 2021

codecov bot commented Apr 1, 2021 •

edited

Matt-Yorkley commented Apr 2, 2021

jibees commented Apr 2, 2021

mkllnk left a comment

Matt-Yorkley commented Apr 22, 2021 •

edited

filipefurtad0 commented Apr 22, 2021

jibees commented Apr 22, 2021

jibees commented Apr 22, 2021 •

edited

Matt-Yorkley commented Apr 22, 2021 •

edited

jibees commented Apr 22, 2021

jibees commented Apr 22, 2021

filipefurtad0 commented Apr 22, 2021

Matt-Yorkley commented Apr 22, 2021

Implements our StyleGuide #7284

Implements our StyleGuide #7284

Conversation

jibees commented Mar 30, 2021 • edited

What? Why?

How it works?

Release notes

What should we test?

Issues I faced:

MonkeyPatch

luisramos0 commented Mar 30, 2021

jibees commented Mar 31, 2021

luisramos0 commented Mar 31, 2021 via email

Matt-Yorkley commented Mar 31, 2021 • edited

jibees commented Mar 31, 2021 • edited

jibees commented Mar 31, 2021 • edited

Matt-Yorkley commented Mar 31, 2021 • edited

jibees commented Apr 1, 2021

codecov bot commented Apr 1, 2021 • edited

Codecov Report

Matt-Yorkley commented Apr 2, 2021

jibees commented Apr 2, 2021

mkllnk left a comment

Choose a reason for hiding this comment

Matt-Yorkley commented Apr 22, 2021 • edited

filipefurtad0 commented Apr 22, 2021

jibees commented Apr 22, 2021

jibees commented Apr 22, 2021 • edited

Matt-Yorkley commented Apr 22, 2021 • edited

jibees commented Apr 22, 2021

jibees commented Apr 22, 2021

filipefurtad0 commented Apr 22, 2021

Matt-Yorkley commented Apr 22, 2021

jibees commented Mar 30, 2021 •

edited

Matt-Yorkley commented Mar 31, 2021 •

edited

jibees commented Mar 31, 2021 •

edited

jibees commented Mar 31, 2021 •

edited

Matt-Yorkley commented Mar 31, 2021 •

edited

codecov bot commented Apr 1, 2021 •

edited

Matt-Yorkley commented Apr 22, 2021 •

edited

jibees commented Apr 22, 2021 •

edited

Matt-Yorkley commented Apr 22, 2021 •

edited