urfuwo / hlx-test

Apache License 2.0
0 stars 3 forks source link

423 external links media variant #455

Closed eduardseifert closed 5 months ago

eduardseifert commented 5 months ago

Fix #423

Test URLs

Before

After

Block

Description

aem-code-sync[bot] commented 5 months ago

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed. In case there are problems, just click a checkbox below to rerun the respective action.

Commits * [7ddeda7](https://github.com/urfuwo/hlx-test/commit/7ddeda7156e91a773bd88ab482a5daa2bad8e4d6) :white_check_mark: (latest) * [a141ea6](https://github.com/urfuwo/hlx-test/commit/a141ea6d608fbc412c4264fb21065c51703d8e2f) :white_check_mark: * [fb2db7c](https://github.com/urfuwo/hlx-test/commit/fb2db7ceed84b7472e5a559c228451b241bc0723) :white_check_mark: * [4b6eea5](https://github.com/urfuwo/hlx-test/commit/4b6eea58de0a25afbd88cb6bda8874217da2e8c0) :white_check_mark: * [f5ceda4](https://github.com/urfuwo/hlx-test/commit/f5ceda4ec583160b5caa246d987cd4db201eaf35) :white_check_mark: * [8b6d8b9](https://github.com/urfuwo/hlx-test/commit/8b6d8b9a077797b9a3864226ba9691157dcc475e) :white_check_mark: * [f6c0415](https://github.com/urfuwo/hlx-test/commit/f6c041539932ffcf14171ad70d7fcef651db7fbf) :white_check_mark: * [e1dec89](https://github.com/urfuwo/hlx-test/commit/e1dec894af64c56e1dc06291880960e22ccb7333) :white_check_mark: * [404ff8b](https://github.com/urfuwo/hlx-test/commit/404ff8b3aea837c84db984e2197fc786bbe3d99c) :white_check_mark: * [1773ce9](https://github.com/urfuwo/hlx-test/commit/1773ce94d08b759ddf14c72dff5b5e8902611306) :white_check_mark: * [025b0f5](https://github.com/urfuwo/hlx-test/commit/025b0f541766d7ea0a21c93061b97d443fea2325) :white_check_mark: * [22278e8](https://github.com/urfuwo/hlx-test/commit/22278e8255fef82080e516b974745a15c5f932b1) :white_check_mark:
aem-code-sync[bot] commented 5 months ago
Page Scores Audits Google
/design/about PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/design/design-system PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/tools/sidekick/blocks/link-list PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/tools/sidekick/library.html?plugin=blocks&path=/tools/sidekick/blocks/link-list
&index=0
PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
saurabh-khare commented 5 months ago

@eduardseifert Congrats on the first PR :) Please provide before and after state for all test Urls so reviewers can check what has changed in the PR. E.g. https://github.com/urfuwo/hlx-test/pull/450 Also for this specific PR please add link to the page which has link with icon in this block.

eduardseifert commented 5 months ago

As discuessed with @mhaack, I will move the icons inside the link, change a few lines of CSS and revert the img handling to be directly embedded SVG's in the markup (DOM) (insted of referencing them in the src attribute).

eduardseifert commented 5 months ago
saurabh-khare commented 5 months ago
Screenshot 2024-04-15 at 15 50 53

Infact we don't need the js at all. I just commented it and the default decorateIcons kicks in because you added icons in the content

Screenshot 2024-04-15 at 15 52 16
saurabh-khare commented 5 months ago

I compared the existing block with the new block and there are many design changes. Please keep the external links block until we get the feature parity in the new block. Make sure existing content and design is not impacted. Reference page: http://localhost:3000/draft/skhare/content-list-test

Screenshot 2024-04-15 at 16 08 01
eduardseifert commented 5 months ago

@mhaack + @saurabh-khare Please have again a look at the change.

  1. Changed to dom-builder.js (thanks for the hint).
  2. I need the additional span for the text to be able to set margin to the left or right (depends on icon position).
  3. I have renamed the block as discussed in Slack to link-list and changed all the necessary files as well. If you want to have it back to links-external, I need to revert it again.