Conversation
| cursor: pointer; | ||
| border-radius: 15px; | ||
| } | ||
|
|
There was a problem hiding this comment.
@Shinh18 : both buttons have the same rules applied, except for the background, you could put everything in common and add two classes on your HTML element, one for the common aspect and one for the specific rule
frontend/src/pages/Book.jsx
Outdated
| <dd>{book.yearPublished}</dd> | ||
| </dl> | ||
| <img src={`https://picsum.photos/200/300`} alt="book cover" width={200} height={300} /> | ||
| <> |
There was a problem hiding this comment.
@Shinh18 You are missing a heading on the page, don't know where we sould put it...
the class names are not really coherent with the rest of the app you should look into the BEM methodology
for exemple : https://webuild.envato.com/blog/how-to-scale-and-maintain-legacy-css-with-sass-and-smacss/
you can also look at how I named in other components.
| @@ -0,0 +1,85 @@ | |||
| @import '../fonts/fontWeight.scss'; | |||
|
|
|||
There was a problem hiding this comment.
don't really like the floats and % unit
i would also like you to change the px unit into em, em is a unit scaled on the current font-size
|
@Shinh18 I guess most of the layout works but i added comments, if something is not clear or you have any issue ask me anytime on discord :) |
|
I made the requested changes to the PR. Please review and let me know. |
2 participants
Fixed #8 : Improved the current style of book info page to match the mockups