Closed LuisSR closed 1 year ago
steffenbew
commented
1 year ago Hey @LuisSR, thanks a ton for your elaborate feedback! That's invaluable! Especially as it's so well documented and constructive. A couple of things are already on our list, some fixed already, others are new and some are conceptional thoughts we'll need to explain or rethink. Bear with us and give us some time to work through that! :)
LuisSR
commented
1 year ago Thank you, @steffenbew! I owe you all an apology for the length of the issue. :laughing:
I'll wait for any update, and I'll be glad to help you if I can.
It wasn't mentioned in the issue, but the theme is amazing!
timohubois
commented
1 year ago @LuisSR many thanks for the feedback and deep dive into Flynt.
I will shortly try to provide an update:
The style.css file has been modified to include a “Requires PHP” line in its header.
The PR is merged and currently we have no known problems with it. If it does not work for you or you figure other issues, let us know or create a new PR with your changes, please.
We have updated the “pageComponents.php” file in relation to the /inc/customPostTypes/example.php, we oriented the code to the output from the GenerateWP Post Type Generator and just want to provide a starting point and we will discuss in the team about adding show_in_rest to this file.
We are aware of this as well and would really appreciate your ideas and improvements in a new PR.
Flynt is following a boilerplate approach and has decided not to make any changes in this area at this time. In the Discussions section, you may also be interested in a thread on legacy support: https://github.com/flyntwp/flynt/discussions/484
That's a really good point, and I hadn't noticed that before. We will open an internal issue to find a solution. Feel free to post a PR as well.
This should be fixed in current master branch.
This should be fixed in current master branch.
This should be fixed in current master branch.
This should be fixed in current master branch.
We decided to keep the current behaviour and give the developer the freedom to do what they want without providing a specific path.
Really interesting topic and approach! We currently feel it should be a developer decision to use this approach or not, so that we decided to not add this feature to the core – at the moment.
We have further compressed the screenshots and updated them to the current master branch. Regarding the logo and the placeholder image, we would be really happy to see you hand optimized svg inside and further adjustments a new PR.
We were really happy to get such kind of a detailed issue and looking forward to your feedback in the future and further suggestions and maybe PRs 🚀
timohubois
commented
1 year ago Closed due inactivity. @LuisSR feel free to create a new issue, provide us more feedback or create a pr at any time!
After some days of testing the new version of Flynt, I have detected a few small issues in the default implementation of the theme.
I don't have enough information about how you work with the theme on client projects. I guess some of the things might not be issues at all but the intended behaviour. Or maybe just code ready to be customized and not taken literally.
Either way, I don't feel confident enough to push any change yet. Some of the solutions I propose require discussion and testing.
This might be a very long issue, but I'll try to structure it the best I can.
I've described each of the issues and provided a fix or the starting point for creating one. I can help you with time-consuming fixes as long as client work doesn't get too busy again.
Unless stated otherwise, all the code has been tested on localhost with WP 6.3 running Flynt 2.0.0 and ACF 6.2.0 over PHP 8.2.4. The browsers are either Firefox 116.0.2 or Opera 101.0.4843.43 under Kubuntu 23.04.
1. Back end issues
1.1. The theme can be activated in servers with a PHP version lower than 7.4 and leave the WP installation unusable
#### Description Activation using an outdated PHP version will render a Composer related fatal error both on the dashboard and on the frontend of the site. Although it's unlikely to happen that a non-technical user downloads and installs Flynt, the fix is so easy I see no reason not to implement it **Possible fix** Just add the requirement as */style.css* headers: ```diff /* Theme Name: Flynt Theme URI: https://www.flyntwp.com/ Description: The lightning-fast WordPress Starter Theme for component-based websites with ACF Pro. Version: 2.0.0 Author: Bleech1.2. Add component button is not working as expected
#### Description In the post edition screen, the "Add component" button is sometimes failing to open the tooltip on the correct position. This has already been pointed out in the following discussion: https://github.com/flyntwp/flynt/discussions/499. It seems to happen when one or more of the components meta boxes are open. **Possible fix** It seems to be a simple tooltip positioning error somewhere in the JavaScript calculations. No errors are shown in console. @aaronmeder provided a working solution, which I have not tested myself: https://github.com/flyntwp/flynt/pull/501 ---1.3. The custom post type boilerplate is showing the ACF components interface, but components are not working on the frontend
#### Description The title pretty much summarizes the issue: when a new post type is registered following the code provided in */inc/customPostTypes/example.php*, the post edition screen shows both the TinyMCE and the Flynt component edition interfaces. However, content written with the latter is not shown in the frontend. To provide some context, I've seen that: * The *single.twig* template handles the view by default, so post_content is used and components won't be retrieved from post meta data. * The block editor is not enabled on the arguments passed to *register_post_type()*. This falls back the editor to TinyMCE, which probably offers the worst experience out of the three possibilities. I also believe that just showing the active editor, whichever it is, and disabling the other two, would be a good UX practice here. #### Possible fix If it were up to me, the default editor for non-componentized post types would be the block editor. It would make a more consistent experience, as this is the way blog posts already work. A single line addition in */inc/customPostTypes/example.php* would do so: ```diff 'show_in_menu' => true, + 'show_in_rest' => true, 'menu_position' => 5, ``` Hiding the ACF components interface could be done by making */inc/fieldGroups/pageComponents.php* more restrictive by default: ```diff 'location' => [ [ [ 'param' => 'post_type', - 'operator' => '!=', + 'operator' => '==', - 'value' => 'post', + 'value' => 'page' ], - [ - 'param' => 'post_type', - 'operator' => '!=', - 'value' => 'reusable-components' - ], ], ], ``` This needs proper testing. ---1.4. Some printed variables via Twig need escaping
#### Description Components that print database values directly into the browser always pose a risk of XSS. Some templates and components output titles and other string values which should be properly escaped: https://github.com/flyntwp/flynt/assets/24281689/c6b47bc6-e55a-4d8c-8bee-0d9d7e1e9297 #### Possible fix I'd suggest reviewing each *.twig* file individually and going case by case. Timber doesn't support *esc_attr()* for escaping tags' attributes, which could come handy, and twig's *escape('html_attr')* is not an exact replacement. Maybe Timber or Flynt can be extended to mimic it. I was thinking also about how to implement a long-lasting automatized solution, but no tool seems to cover Twig escaping. There is a Linter in Symfony and a kind of CodeSniffer for Twig, but it's a non-maintained package. ---2. Front end issues
2.1. Support for Safari v14 and lower is not complete
#### Description Safari 13 and 14 don't support some modern CSS properties which are used for core parts of the theme, like the masthead, being the most notable *padding-block*:  Devices using Safari 14 might include MacOS and iOS devices bought before the T4 of 2021 which have been updated on their current release branch or have not been updated at all. Even the newest version of the 14 branch has issues:  Devices like the iPhone 12 Pro are just two generations old high-end hardware. And pretty much any blink-based browser prior to the end of 2020 will show layout problems:  I am pretty conservative regarding browser support, so I understand this might not be considered an issue at all, but for me the dates and devices involved are still recent. #### Suggestion Sadly, I fear this is something the autoprefixer PostCSS plugin won't be able to handle, even if *"ios 13, Safari 13"* are inside the browserlist support. A full fix is out of the scope of this issue. It would mean manually reverting many properties on quite a few selectors and affect the whole styling of the header and the horizontal and vertical spacing on several component views. But I'll be happy to help with this task if you decide to implement it. ---2.2. Posts created with the core/classic block or with the TinyMCE editor do not have their content wrapped
#### Description In both cases, the issue is the same: failing to constrain and center the content.  Classic blocks are the expected result of importing html content from older WP installations or from another CMS. Apart from potential migration problems, WordPress editors who still prefer to use TinyMCE via the *Classic Editor* or *Disable Gutenberg* plugins will face the same problem. WordPress won't convert even perfectly clean html into blocks straightaway. There were some options available for bulk converting the classic blocks from all posts into real blocks, but none is in a working state right now. #### Fix The block editor adds inline styles for limiting the width of blocks without alignment options: ```css body .is-layout-constrained > :where(:not(.alignleft):not(.alignright):not(.alignfull)) { max-width: var(--wp--style--global--content-size); margin-left: auto !important; margin-right: auto !important; } ``` By default, WordPress injects these styles into the markup of singular templates, whether it's a page, a post or a CPT post. But Flynt programmatically removes the so called "global styles" in */inc/blockEditor.php* for any view that has no blocks inside the_content. In the case of a post created with TinyMCE it's pretty clear that the chosen validation will fail. But the reason why the *core/classic* block is not triggering *has_blocks()* might not be that intuitive. The reality is that the classic block is actually not a block but a virtual wrapper. **Short way fix**: Change the validation performed in */inc/blockEditor.php* for removing the global styles: ```diff /** * Remove Gutenberg block related styles on front-end, when a post has no blocks. */ add_action('wp_enqueue_scripts', function () { - if (!has_blocks()){ + if (is_page()) { wp_dequeue_style('core-block-supports'); wp_dequeue_style('wp-block-library'); wp_dequeue_style('wp-block-library-theme'); wp_dequeue_style('global-styles'); } }); ``` Note that *is_page()* assumes that only pages will have Flynt components. This might not be true for all or some custom post types on more complex projects. Ideally, I'd suggest creating a helper function to check if a singular post has been created with components instead. **Long way fix**: Disabling global styling everywhere and hard-coding into Flynt the wrapper width and a few more things (basically spacing) that are handled by the inline styles. This is easier said than done, but will have the benefit of not inheriting any future, potentially design-breaking style that upcoming WordPress releases might bring. ---2.3. Small styling issues around the post header and meta
#### Description * **2.3.1.** An extra divider is shown in the post meta when a post has no categories Although it's not possible that a post has no associated category under normal usage, this behaviour is not mandatory for custom post types. Also, some content importing tools can programmatically create posts without the default category.  In these cases, the divider shouldn't be shown. * **2.3.2.** Author box and categories/reading time are not correcty aligned  * **2.3.3.** Bleeding is missing when a post has no featured image  #### Possible fix For **2.3.1.**, a simple check in Components/BlockPostHeader/index.twig for existing post categories is needed: ```diff + {% if post.category %} {{ labels.postedIn|e }} {{ post.category }} + {% endif %} ``` **2.3.2.** is a problem of mixing different font-sizes. I'd define a font-size property for all meta elements to inherit: ```CSS flynt-component[name=BlockPostHeader] .meta { font-size: var(--font-size-body-small); } ``` The problem with **2.3.3.** is that the bleeding is attached to the featured image component (```flynt-component[name=BlockPostHeader] flynt-component[name=BlockImage] {...}```). I haven't tested it, but maybe it could be in the main content section instead (```.wp-block-post-content```). ---2.4. Refine the aria-label on components that show a grid of posts
#### Description Some components output post queries into cards. The whole post card is a link ("a" tag) that has an aria-label attribute assigned to the post title. For improved accessibility, this should start with something like *"Read the article [post title]"* rather than just *"[post title]"*. This is also known for triggering a warning on the accessibility score of the PageSpeed Insight tests. If I'm not mistaken, the criterion to apply would be this one: https://www.w3.org/WAI/WCAG22/Understanding/link-purpose-in-context The affected templates are: * /Components/GridPostsArchive/index.twig * /Components/GridPostsLatest/index.twig * /Components/ListSearchResults/Partials/_results.twig: #### Possible fix It's a one line fix: ```diff - + ``` ---3. Suggestions
The following points are not issues at all, but performance or usability improvements you might want to consider.
3.1. The default block editor options might be overwhelming for content editors
#### Description This is opinionated, but the block editor provides many options for building layouts which are not useful for blog or news posting. On one side, many blocks, such as "core/query", "core/comments", "core/navigation", "core/archive-title" or "core/rss", are intended for complex page layouts or for the Site Editor (FSE) templates. They will never be part of any real content writing experience, and some are even not working properly in regular posts. On the other side, WP 6.3 has almost one hundred block types, many of which serve the same purpose but with slightly different styles. Should one use "verse", "pullquote" or "blockquote"? Or a "column + text/column + image" or just "media-text" block? When this styling decision is on the hands of the client employees or the marketing content editors, posts tend to end having different visual styles depending on their authors. Having so many useless blocks bloats the block selection screen and worsens the content editor experience. #### Suggestion **3.1.1.** Limit the available blocks for the post type "post". This can be done via the */theme.json* file or within */inc/blockEditor.php*. I, personally, prefer to opt for the most constrained experience (negating all blocks) and then decide case by case those that will be allowed: ```php /** * Control which block types are available by default. */ add_action('allowed_block_types_all', function ($allowed_block_types, $block_editor_context) { if (!isset($block_editor_context->post->post_type)) : return $allowed_block_types; endif; switch ($block_editor_context->post->post_type) : // Blog posts case 'post': $allowed_block_types = [ 'core/classic', 'core/gallery', 'core/heading', 'core/html', 'core/image', 'core/list', 'core/list-item', 'core/paragraph', 'core/quote', 'core/table', 'core/video', ... ]; break; // Other registered post types default: break; endswitch; return $allowed_block_types; }, 10, 2); ``` But it's also possible to remove just the unwanted blocks, while keeping future core blocks support open by default: ```php /** * Control which block types are available by default. */ add_action('allowed_block_types_all', function ($allowed_block_types, $block_editor_context) { if (!isset($block_editor_context->post->post_type)) : return $allowed_block_types; endif; switch ($block_editor_context->post->post_type) : // Blog posts case 'post': $registered_blocks = \WP_Block_Type_Registry::get_instance()->get_all_registered(); unset($registered_blocks['core/query']); unset($registered_blocks['core/comments']); unset($registered_blocks['core/rss']); ... $allowed_block_types = array_keys($registered_blocks); break; // Other registered post types default: break; endswitch; return $allowed_block_types; }, 10, 2); ``` **3.1.2.** If blocks are limited to just a few, for improved performance and visual consistency, I'd consider dequeueing the default block editor stylesheet: ```diff /** * Remove Gutenberg block related styles on front-end, when a post has no blocks. */ add_action('wp_enqueue_scripts', function () { if (!has_blocks()) { wp_dequeue_style('core-block-supports'); wp_dequeue_style('wp-block-library'); wp_dequeue_style('wp-block-library-theme'); wp_dequeue_style('global-styles'); + wp_dequeue_style( 'wc-block-style' ); + wp_dequeue_style( 'classic-theme-styles' ); } }); ``` Base blocks, such as paragraphs or headings, will inherit Flynt's styles. But others, like *core/buttons* will need manual styling. This might be a bit of work at the beginning, but it will also help to match the posts visual styling with the design system of a given site. Alternatively, the default block editor styles can be imported from the scss files available at the Gutenberg project repository: https://github.com/WordPress/gutenberg/tree/trunk/packages/block-editor/src. If point 3.1.2. ever makes it into Flynt or a project made around it, I'd recommend you to opt for the restrictive approach. Controlling the styles and no depending on third-party CSS is much more consistent and future-proof. The block editor has changed the styling approach several times during these years, and many of these changes have had an impact on the frontend of the websites. This is particularly undesirable on corporate websites. ---3.2. Group CSS media queries
#### Description Converting this (unminified for readability) ```css @media (min-width: 768px) { .flynt-component[name='ComponentA'] .container { property1: value1; } } @media (min-width: 768px) { .flynt-component[name='ComponentB'] .container { property2: value2; } } ``` into this ```css @media (min-width: 768px) { .flynt-component[name='ComponentA'] .container { property1: value1; } .flynt-component[name='ComponentB'] .container { property2: value2; } } ``` could save some KBs for each page view on every site made with Flynt at virtually no cost. #### Suggestion There is a PostCSS plugin that can perform the grouping automatically: https://www.npmjs.com/package/postcss-combine-media-query I added it to the vite.config.js and then I built for production, resulting in a 3.34% smaller stylesheet. On a bigger project, with several components tailored both for mobile and desktop devices, this percentage can be as high as 15% or 20%. One prevention: this won't run in the development environment, and moving the position of media queries implies a small risk of inconsistency if the CSS is badly structured. In my experience, after years of using this technique in production, I haven't run into a single issue. But I'd double-check! ---3.3. Included assets can be further compressed
#### Description From the logo to the miniatures of each block, some KB can be saved here and there: | File | Original size | Compressed size | | --- | --- | --- | | /assets/images/logo.svg | 916 | 778 | | /Components/BlockAnchor/screenshot.png | 7411 | 6191 | | /Components/BlockImage/screenshot.png | 6861 | 4767 | | /Components/BlockImageText/screenshot.png | 6985 | 5106 | | /Components/BlockNotFound/screenshot.png | 5354 | 3991 | | /Components/BlockPostFooter/screenshot.png | 5826 | 4230 | | /Components/BlockPostHeader/screenshot.png | 7710 | 4842 | | /Components/BlockSpacer/screenshot.png | 8383 | 5802 | | /Components/BlockVideoOembed/screenshot.png | 6643 | 3645 | | /Components/BlockWysiwyg/screenshot.png | 4578 | 2947 | | /Components/FeatureFlexibleContentExtension/screenshot.png | 16568 | 8066 | | /Components/FormPasswordProtection/screenshot.png | 4865 | 2551 | | /Components/GridImageText/screenshot.png | 7521 | 5430 | | /Components/GridPostsArchive/screenshot.png | 7975 | 7111 | | /Components/GridPostsLatest/screenshot.png | 7719 | 5292 | | /Components/ListComponents/screenshot.png | 6943 | 4334 | | /Components/ListSearchResults/screenshot.png | 5052 | 3551 | | /Components/NavigationBurger/screenshot.png | 8466 | 5038 | | /Components/NavigationFooter/screenshot.png | 4519 | 3638 | | /Components/NavigationMain/screenshot.png | 8077 | 4683 | | /Components/ReusableComponent/screenshot.png | 22934 | 11818 | | /Components/SliderImages/screenshot.png | 7299 | 4527 | It doesn't make much of a difference (about a 5.6% of the whole uncompressed theme size), but can have a positive impact in countries with slow internet connections. The svg file was compressed by hand. The png files with pngquant. */lib/Utils/TwigExtensionPlaceholderImage.php* is also converting the image placeholder used for lazy-loading to a base64 data-uri, when 'data-uried' svg are known to be shorter in size and gzip better with proper encoding of symbols instead of base64 the whole image. You can have a look at this library for more info: https://github.com/tigt/mini-svg-data-uri/tree/master. This is the only image that is shown in the front end (apart from Flynt's own website) and the gain is quite small. But it's so widely used (each image on every page on every site made with the theme) that might be worth digging into it. ---