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

Features/landing promo #27

Merged
merged 38 commits into from
May 28, 2019
Merged

Features/landing promo #27

merged 38 commits into from
May 28, 2019

Conversation

viglesias
Copy link
Contributor

@viglesias viglesias commented Mar 14, 2018

Carga Lazy de bloques.

@viglesias viglesias requested a review from brenes March 14, 2018 10:38
Copy link
Member

@brenes brenes left a comment

Choose a reason for hiding this comment

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

First of all, great PR!

I see four more things in this PR that should be fixed:

  1. We should ensure this change is backwards compatible. That's done by checking all specs are green without having to change them but CircleCI is red.

  2. There are no specs testing the new skeleton and CSS options. Tests we should include (at least):

  • The block is detected as a lazy or normal block.
  • The same block in different templates can be detected as a lazy or normal block.
  • When a block is lazy it prints the skeleton.
  • The CSS files and mediaqueries are correctly returned when asked.
  1. There's no documentation on the README. It changes a lot of configuration and it should be explained in the README. In the README should be included that the "lazy mechanism" of how to load a lazy block is up to the app, no this gem. This option is only included so the app can easily include it without having to rewrite all the helpers.

  2. CSS files can be included in the asset pipeline by reading the configuration. We should include an assets initializer to include all the detected CSS files in the configuration to the asset pipeline. This way, the programmer doesn't have to include it manually (and forget)

app/helpers/no_cms/blocks/blocks_helper.rb Outdated Show resolved Hide resolved
app/helpers/no_cms/blocks/blocks_helper.rb Outdated Show resolved Hide resolved
lib/no_cms/blocks/configuration.rb Outdated Show resolved Hide resolved
app/models/no_cms/blocks/block.rb Outdated Show resolved Hide resolved
Copy link
Member

@brenes brenes left a comment

Choose a reason for hiding this comment

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

Another test we should include, the generators generate this new file

brenes and others added 18 commits March 15, 2018 15:46
This should also fix the travis suite as the generator spec was dailing because od the missing default skeleton
We fix the case were there's no valid mediaquery for the CSS file
These specs have arisen one issue: There's no lazy blocks option in the general section of a template.
Until now, lazy blocks were only checked with zone configurations, not with the template global configuration
@brenes brenes merged commit d8dc09f into edge May 28, 2019
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.

None yet

2 participants