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

#108 Add stylized dogs#index page #112

Merged
merged 6 commits into from
Aug 25, 2022
Merged

#108 Add stylized dogs#index page #112

merged 6 commits into from
Aug 25, 2022

Conversation

edwinthinks
Copy link
Collaborator

@edwinthinks edwinthinks commented Aug 24, 2022

Resolves Issue #108 & partly #106

This PR adds the dogs#index page with styling to match the mock-up. Pull this branch down and goto the "/dogs" page.

Here are some high-level details worth noting:

  • Added the nice_partials gem which allows us to enhance the partials by allowing you to define "slots". This is possible without the library but isn't as clean IMO.
  • Took some styling liberties to move the navigation to the right side and space them out.
  • This PR doesn't utilize any "real" dog data, it generates it randomly upon load. Enjoy the pics!
  • Utilizing a component-based approach which I've found works well with Tailwind. That is you can see that a components/_header and components/_textbody partial was added so we can keep the styles consistent and avoid duplication. Changing the individual partials will change the page styling. This was only used on components that I actually think we be reused

Note

  • I usually add specs but didn't really feel the need for it yet! We should certainly investigate adding some test/specs in the near future for this new page.
  • I didn't figure out what font this is, so this PR is somewhat incomplete without the correct font.

Here's a demo picture of it in desktop & mobile view:
Captura de Pantalla 2022-08-23 a la(s) 8 14 07 p m
Captura de Pantalla 2022-08-23 a la(s) 8 14 37 p m

@edwinthinks
Copy link
Collaborator Author

Interesting the spec locally doesn't fail for me, but it is failing in CI.... Hmmm

@ejuten
Copy link

ejuten commented Aug 24, 2022

Interesting the spec locally doesn't fail for me, but it is failing in CI.... Hmmm

I found this: tailwindlabs/tailwindcss#6738

Looks like if you add config.assets.css_compressor = nil to the config/environments/production.rb file it may solve the issue.

@edwinthinks
Copy link
Collaborator Author

@ejuten I can try that. Though I'am not entirely sure why this is causing an issue since the test should be running on the test environment.... hmm I can try this in test.

@ejuten
Copy link

ejuten commented Aug 24, 2022

@edwinthinks solid point. shrug

I rewrote what you just put above. I should go to bed.

@edwinthinks
Copy link
Collaborator Author

@ejuten looks like it worked. Let me see if it has to do with the NODE_ENV though, maybe its trying to compile production but should be test. Lemme try that.

@ejuten
Copy link

ejuten commented Aug 24, 2022

What an odd issue, this comment is interesting and maybe what the issue is? Not sure if it's worth it if the compressor line works?

Ok, so by default rails (6) comes with gem 'sass-rails' and app/assets/stylesheets/application.css.scss. 
In my case this file had only *= require_self. The rabbit hole took me down to sassc-rails, 
which depends on sassc, which depends on LibSass, which tends to be behind with features 
and everyone moved to Dart.

I actually don't need sass as in my case everything is being handled through jsbundling-rails with 
esbuild that uses sassPlugin and postcss. After I dropped it from gemfile and removed 
application.css.scss doing rake assets:precompile passed.

Other people who can't drop sass-rails may not be so fortunate.

Thx adam for pointing me the right way.

@edwinthinks
Copy link
Collaborator Author

@ejuten yesss! It works. So I removed sass-rails as per the comment you shared since I saw we only used it for one file and it wasn't even using .scss syntax.

All green :)

@kinduff
Copy link

kinduff commented Aug 24, 2022

Does this also cover #103? I'm looking to contribute to that page

@edwinthinks
Copy link
Collaborator Author

@kinduff kind of - this doesn't handle any of the data modelings of dogs yet which I think is a requirement.

@freestylebit
Copy link
Collaborator

freestylebit commented Aug 24, 2022 via email

Copy link
Collaborator

@chriszimmerman chriszimmerman left a comment

Choose a reason for hiding this comment

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

This looks fantastic! 🎉

@edwinthinks edwinthinks merged commit 9223a11 into main Aug 25, 2022
@edwinthinks edwinthinks deleted the styled-dogs branch August 25, 2022 23:05
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.

5 participants