Skip to content

Conversation

@andapop
Copy link
Contributor

@andapop andapop commented Nov 16, 2022

  • added reset
  • moved template from ul, to be able to use :empty pseudo element to display the loading message
  • added grid for cards and updated css a bit

did not format the files so we can more easily compare the changes, but i have the formatted versions - I would suggest that we use the default prettier settings

@codesandbox
Copy link

codesandbox bot commented Nov 16, 2022

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

*,
*::before,
*::after {
box-sizing: border-box;
Copy link
Contributor

Choose a reason for hiding this comment

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

we should add this to the starter as well, not just in the final solution. it's actually a great point to discuss with beginners, I remember when I started with html/css this was one of the things that always confused me

Copy link
Contributor

@pogadev18 pogadev18 Nov 17, 2022

Choose a reason for hiding this comment

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

@andapop feel free to make some of these changes also in this branch starterImprovement that has an open PR. We can add pieces of this CSS also for the starter, just like @alexnm said, and also we can update the HTML with your new changes. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

UPDATE: starterImprovement branch is now merged so maybe we can create a new branch for the above comment

src/style.css Outdated
this example is useful for a React app that would render in a div with id root
*/
#root {
isolation: isolate;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would skip some of these in the starter, to make sure we don't overwhelm people with things they don't know about/don't need for this intro.

We can use it as a reset for the react-starter though, even in a separate file or something

src/index.html Outdated
</section>
<section>
<h2 class="u-screen-reader-text">Repositories</h2>
<!-- remove content from ul, so we can use :empty to display the loading
Copy link
Contributor

Choose a reason for hiding this comment

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

I love this :)

@alexnm alexnm merged commit d702544 into finalSolution Nov 17, 2022
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.

4 participants