-
Notifications
You must be signed in to change notification settings - Fork 27
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
refactor: add app routing and lazy load home feature #23
refactor: add app routing and lazy load home feature #23
Conversation
Deploy request for ngx-darija rejected. Rejected with commit 332f310 https://docs.netlify.com/configure-builds/environment-variables/#sensitive-variable-policy |
Hi @Mubramaj Did you remove if that is the right behavior of Angular, that's a good info to know 😉 |
Hi @ikourfaln, No I removed ViewEncapsulation.None to keep default behaviour of Angular which encapsulate each component
Then only the links in the I also removed empty scss files just to clean up project. I hope it make sense, let me know what you think |
thank you @Mubramaj for reply Yes, I already know the behavior of ViewEncapsulation.None, but because we know that we will work only with Tailwind css (already added to the project), there is no need for encapsulation, even Tailwind css community advice that, except if you are forced to work with style file, bcz it helps to avoid some issues, plus there no need to start the preprocessor of the component style (Encapsulation) if it's empty. that's why I asked you if Angular is smart enough to ignore the Encapsulation if style file is empty. |
@ikourfaln my bad, I never worked with Tailwind css. I will add back encapsulation None. So if I want to set a css class to use in several parts in the app where should I add this class ? (usually I do it in Also if I want to define css styles encapsulated for only 1 component, how should I do it with Tailwind ? |
@Mubramaj no problem Tailwind css has almost all you need (layout, spacing, colors, responsive...etc) as utility classes. So if you have really a specific style need you wanna use it in several parts in the app, you can do it in And if you are enforced to style a component and encapsulated it, you can remove nothing stops you to do what you want, it's just better to follow best practices and conventions 😉 PS: you can use Tailwind css plugin for VS Code, it really helps |
@ikourfaln thanks for the answer. Ok cool, good to know. I used For the css classes of Tailwind, I usually use bootstrap and they have utility classes so I figured Tailwind also had the same thing. I looked at their doc. |
You are welcome @Mubramaj (your use case (spacing), you can use https://tailwindcss.com/docs/padding, https://tailwindcss.com/docs/margin, or https://tailwindcss.com/docs/space) PS: Tailwind and Bootstrap have different philosophy |
Thanks for this awesome PR @Mubramaj! Data should always be available because the site is statically generated at build time (unless the request fails at build time which should/would also be handled). I am not sure whether we should display the loading spinner when the Client side request is made, because data is already here, currently there should be no update of the DOM unless some data has changed on the Playlist as in the following scenario: After deploying your branch here: https://604d4d75da06b400080e7097--ngx-darija.netlify.app I removed "Salam" from the descriptions. You can see the update on the screen after the first display with "Salam". After updating the playlist, we should usually trigger another build to pre-render with lastest data. |
Hi @chihab , If I understand correctly, spinner not needed because first request already have static data with the playlists. My idea behind the spinner was the following: I am going to fix the conflicts, let me know if I should remove spinner for now since we only have 1 page |
@Mubramaj |
@ikourfaln Good catch, will do that later tonight ! |
Done ! @chihab I removed the spinner since we create static file at build time. It should be good to go unless there is more feedback ;) |
Thanks for the update @Mubramaj! |
Changes in this pull request
I think there is a little flickering the first time we render the page when using SSR.
I think this is due to the first
pre-rendering
content, then component run in browser, displays the spinner, fetch data and render it again.