First Review #1
First Review #1
Conversation
|
@lewis-dbi Oh man, this really looks great. The thing I love the most is that I didn't find one thing out of place when looking at it in the browser. In my experience, that's been most of the battle I've had reviewing frontend work. Devs just don't see the inconsistencies between the mockups and what their HTML+CSS is producing, so it's really great not to have screenshot things and point out the subtle differences. I've created this Pull Request so that I can add comments to specific lines of code and we can discuss them and loop Pete in as well. I'll submit a review soon. |
Added gitignore to exclude node_modules and compiled assets
For SVG minifying and sprite sheet
This is for layout only styling to display the components on the page including labels and margins.
Removed layout only styling and amended CSS as per feedback
Added inline SVGs using a spritesheet, added buttons tags to all components that required them and other feedback applied.
Removed unused dependencies
|
@bradt Ready to review. I made quite a few changes.
|
Added a .wpmdb class so that the styling doesnt get overwritten in the WP dashboard
Fixed a few errors and adjust some items for the WP dashboard.
Stirctly for laying out the components on the page. Wrapped inside a .wpmdb tag
Sass added to this project and this is the CSS file wrapped inside .wpmdb tag
This styling overrides some other styling when placed inside the WP dashboard.
|
@bradt All updated and as close as I can get it inside the WP dashboard. I will send you a zip file separately on Monday so you can how it looks inside the WP dashboard. One thing I'm still not sure about is if we should set a min-width to a whole wrapper tag that sits around the whole codebase or just on individual components like the accordions and the nav etc. Also, using SASS now too. Although it's basically my CSS nested inside a .wpmdb tag. |
|
This looks great Lewis, nice work! 🎉 Only a few minor things for you to review here. I expect that Pete will have some more pointed feedback for you once he starts implementing things. |
|
Once you've finished with this round of feedback, please give Pete the nod to start implementing. |
Added contents to styles.scss
This reverts commit feaedc3.
|
@ptasker Ready for you to use. |
Just testing out where my avatar now shows when doing a commit
|
|
||
|
|
||
| <!--Search and Replace--> | ||
| <table> |
There was a problem hiding this comment.
In my current markup I'm avoiding using the table element as it's not really a dataset, which is what <table> is typically used for these days.
There was a problem hiding this comment.
@bradt Any ideas for this? I replaced the
tag with a div and it breaks the layout. I tried a few different things but couldn't get it to work.There was a problem hiding this comment.
Again, I don't think semantics matter much here, so it's really whatever HTML and CSS are the best fit for the job. @ptasker What's your suggestion?
There was a problem hiding this comment.
In the placeholder markup I'm using CSS Grid with React's styled components with a wrapper div as below:
export const Row = styled.div`
display: grid;
margin: 0.4rem 0;
grid-template-columns: 0.5fr 5fr 0.5fr 5fr 0.5fr;
row-gap: 10px;
column-gap: 10px;
.header-2{
grid-column: 2
}
.header-4{
grid-column: 4
}
`;This guy wraps 5 divs, and it creates the same layout as a table. Much less markup.
https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Grid_Layout
I can make the styles you have here work with the existing markup I have, so no worries updating this.
There was a problem hiding this comment.
@ptasker Yes please. Then I'll have a look at it so I know for next time.
There was a problem hiding this comment.
@lewis-dbi Maybe you ought to convert the component library from Flexbox to CSS Grid? Sounds like something for you that's worth learning.
This rules are now in the main style.scss file
Removed duplicate css and made some other amends as per Pete's feedback
Amended some of the HTML as per Pete's feedback. This includes remove some class names and targeting that element instead.
Created a class inside of the sass file, and then nested .td classes insidethe corresponding class. decided not to name classes as they are pretty arbitrary.
No description provided.