Featured posts block #22
Conversation
| @@ -21,5 +21,13 @@ $menu-vertical: ( | |||
| padding: map-get-strict($menu-vertical, link-padding); | |||
|
|
|||
| @include link($menu-vertical, link-modifiers); | |||
|
|
|||
| &:hover { | |||
| color: $base-black-color; | |||
There was a problem hiding this comment.
never put external variables direct like in te previouse PRs
| <hr /> | ||
|
|
||
| <ParagraphOptions | ||
| {...attributes} |
|
|
||
| display: -webkit-box; | ||
| -webkit-line-clamp: 2; | ||
| -webkit-box-orient: vertical; |
There was a problem hiding this comment.
we have autoprefixer for this so there is no need to write vendor specific css
There was a problem hiding this comment.
This is an odd case where every browser vendor made it work only with -webkit prefix.
I changed it later to something more standard, though.
There was a problem hiding this comment.
if you have something in css that is only for one vendor and it is not supported in other, you don't use it.
| &__excerpt { | ||
| margin-bottom: 24px; | ||
|
|
||
| display: -webkit-box; |
There was a problem hiding this comment.
(same thing as above)
| display: -webkit-box; | ||
| -webkit-line-clamp: 2; | ||
| -webkit-box-orient: vertical; | ||
| overflow: hidden; |
There was a problem hiding this comment.
why is there overflow on heading?
There was a problem hiding this comment.
Partly same thing as above, and partly because I want to make all cards have equal height, so I needed to make title height fixed so it's not dependant on the post title length.
| } | ||
|
|
||
| &__image-wrap { | ||
| height: 60%; |
There was a problem hiding this comment.
this is highly specific what does this do?
There was a problem hiding this comment.
That was something I tried before I realized there was a bug with columns in Safari which your PR later fixed.
Looks like I forgot to include one of the commits, that'll be solved now.
| <?php | ||
|
|
||
| /** | ||
| * Template for the Card Component. |
There was a problem hiding this comment.
if you component is called Post Card the the doc comment must say the same
| @@ -0,0 +1,91 @@ | |||
| <?php | |||
There was a problem hiding this comment.
if you are not using this remove it
| <div class="<?php echo esc_attr($blockClass); ?>" data-items-per-line=<?php echo \esc_attr($itemsPerLine); ?>> | ||
| <?php | ||
| $request = new \WP_REST_Request('GET', '/eightshift-libs/v1/news'); | ||
| $request->set_param('_limit', 4); |
There was a problem hiding this comment.
not a big fan of this implementation. You are throttling the whole site until Rest request is resolved.
I think this implementation should stay the same as it was from the libs and just provide the Load more component necessary operators to make a query on load more using JS
| @@ -0,0 +1,53 @@ | |||
|
|
|||
| async function load() { | |||
There was a problem hiding this comment.
please rewrite this our way as I mentioned in the previous comments
|
|
||
| for (let element of elements) { | ||
| const url = element.dataset.url; | ||
| const button = element.querySelector(`${selector} ${manifest.componentName}__button`); |
There was a problem hiding this comment.
never put js functionality on the BEM element
|
|
||
| console.log(element.dataset); | ||
|
|
||
| let container = document.querySelector(`${selector}--container`); |
There was a problem hiding this comment.
never put js functionality on the BEM element
|
|
||
| export const FeaturedPostsEditor = ({ attributes, setAttributes }) => { | ||
| return ( | ||
| <LoadMoreEditorComponent |
There was a problem hiding this comment.
why is FeaturedPostsEditor loading only LoadMoreEditorComponent?
Where is the part for the FeaturedPostsEditor?
There was a problem hiding this comment.
It's only LoadMoreEditorComponent because I didn't have anything extra to add for Featured Posts, but I still wanted the editable button text from the Load More editor
| ?> | ||
|
|
||
| <?php | ||
| echo wp_kses_post( |
There was a problem hiding this comment.
as I see you are loading initial content using JS correct?
If yes then this is not the best approach because this way google will not find anything on the page regrading this content so it is bad for the Seo.
You initial content should be loaded using server side rendering (php) and load more should as it is named. Load more
There was a problem hiding this comment.
Yup, I'm loading it with JS.
What's the best way to render it server-side without throttling the site with a REST request?
I can't really use WP_Query like in the stock Featured posts block because I'm fetching data from the local REST api, not from the DB.
No description provided.