Use CSS Modules for page-list component #414
Conversation
|
No worries, in the future, you need not close out your PR, you can just push up a new commit that fixes the test. We all break CI, it's a part of life! |
There was a problem hiding this comment.
Thanks for working on this, @jatinAroraGit! I made a few notes inline — there’s one thing that’s not working right and a few places you could reduce repetition.
In future pull requests, could you please use a more descriptive title? I already updated the one here for you. :)
Also, if one of your commit messages includes the text fixes #407 (instead of Fixes Issue 407), GitHub will automatically link this PR to the issue, and it will auto-close the issue when we merge your PR. You can read more about that here: https://help.github.com/en/articles/closing-issues-using-keywords
| /* ===== List View ===== */ | ||
| .page-list { | ||
| table-layout: fixed; | ||
| } |
There was a problem hiding this comment.
Can you please make sure the indentation is consistent for all these style rules?
There was a problem hiding this comment.
Also, this .page-list rule is not actually being applied anymore! That’s what causing this ugliness on small screens:
In line 50 of page-list.jsx, you need to have:
<table className="table" styleName="page-list">Instead of:
<table className="page-list table">Note that the .table class comes from bootstrap, so it needs to stay in the className attribute instead of changing to styleName.
| background-color: #eee; | ||
| } | ||
|
|
||
| .page-list-td { |
There was a problem hiding this comment.
You can still use descendant selectors with CSS modules, so you could also write:
.page-list td {or:
.page-list > tbody > tr > td {That would save you from having to repeat .page-list-td several times down in lines 79-82 of page-list.jsx.
You could do something similar for the .page-list-tbody-tr classes you have above and the .page-list-th classes below.
| <th styleName="page-list-th" data-name="capture-date">Capture Date</th> | ||
| <th styleName="page-list-th" data-name="site">Site</th> | ||
| <th styleName="page-list-th" data-name="page-name">Page Name</th> | ||
| <th styleName="page-list-th" data-name="url">URL</th> |
There was a problem hiding this comment.
See comment in the CSS file about changing the selectors so you don’t have to repeat this so many times.
| <td styleName="page-list-td">{record.latest ? dateFormatter.format(record.latest.capture_time) : 'No saved versions'}</td> | ||
| <td styleName="page-list-td">{formatSites(record.tags)}</td> | ||
| <td styleName="page-list-td">{record.title}</td> | ||
| <td styleName="page-list-td"><a href={record.url} target="_blank" rel="noopener">{record.url}</a></td> |
There was a problem hiding this comment.
See comment in the CSS file about changing the selectors so you don’t have to repeat this so many times.
|
Hey @Mr0grog , I updated the code to address all the review comments. Thanks for guiding through. |
| @@ -0,0 +1,33 @@ | |||
| /* ===== List View ===== */ | |||
There was a problem hiding this comment.
I think you can remove this comment at the top since the CSS is now scoped at the component level, thanks!
| table-layout: fixed; | ||
| } | ||
|
|
||
| .page-list>tbody>tr { |
There was a problem hiding this comment.
Can you add a space between each selector like so? .page-list > tbody > tr
Same for each of the styles below.
|
|
||
| .page-list>th[data-name="url"] { | ||
| width: 40%; | ||
| } |
There was a problem hiding this comment.
Can you add a newline at the end of the file?
There was a problem hiding this comment.
You can have this formatted for you automatically if you add the EditorConfig plugin to your text editor: https://editorconfig.org
|
@jatinAroraGit Just checking in — are you still working on addressing the last few issues @SYU15 commented about? |
|
Hey @Mr0grog , I will start working again on this , this weekend. Sorry for the delay as it has been an exam week. |
|
No worries! Was just making sure. 😄 |
|
Hey @Mr0grog , did a commit addressing the requested changes. Let me know if there are any issues. |
I am resubmitting the pull request with testing passed.
Sorry for any mistakes I might have done. This is my first ever contribution to an external project in open source.
Thanks.
Fixes #407.