Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

250 and 251: author, topic and tag pages #258

Merged
merged 8 commits into from
Mar 14, 2024
Merged

Conversation

pahupe
Copy link
Contributor

@pahupe pahupe commented Mar 12, 2024

fix: non-curated and curated author pages (#250)
fix: non-curated and curated topic and tag pages (#251)

Author Pages - four examples:

Topic pages:

Tag pages:

NOTE: None of the author pages render as expected (at least for me), despite having published the pages and the author index:

Content changes:

Default vs override:

  • Default for L2 pages is: there is a side nav per default (default as required by design team); pages can choose to have no sidenav

Fix #250
Fix #251

Test URLs:

- Non-curated, dynamically generated author page:
/author/jacquelineprause
- Curated, full author page, custom sidebar ("Julia White"): /author/juliawhite
- Curated, full author page, no sidebar: /author/scottrussell
- Curated, simple author profile, default author sidebar ("Primary Contributors"): /author/luistrunkdeflores
@pahupe pahupe linked an issue Mar 12, 2024 that may be closed by this pull request
Copy link

aem-code-sync bot commented Mar 12, 2024

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.

  • Re-run PSI checks
  • Re-sync branch
Commits

@pahupe pahupe changed the title 250 author pages 250 and 251: author, topic and tag pages Mar 12, 2024
@mhaack
Copy link
Collaborator

mhaack commented Mar 12, 2024

@pahupe with #270 the Title Banner (author) block should go away and replaced by a hero. WDYT?

@aem-code-sync aem-code-sync bot temporarily deployed to 250-non-curated-author-pages March 12, 2024 15:31 Inactive
@pahupe
Copy link
Contributor Author

pahupe commented Mar 12, 2024

@pahupe with #270 the Title Banner (author) block should go away and replaced by a hero. WDYT?

@mhaack: Fine with me. On the "full author profile" pages like Julia White and Scott Russell, I had replaced the existing Columns block with a Hero, see

I am fine to also remove the Title Banner on the "limited profile" and "dynamically generated" (author template) author pages.

@pahupe
Copy link
Contributor Author

pahupe commented Mar 12, 2024

@pahupe with #270 the Title Banner (author) block should go away and replaced by a hero. WDYT?

@mhaack : But let's not throw away the Title Banner block, as we still need it for Topic and Tag pages

@benpeter
Copy link
Collaborator

@pahupe with #270 the Title Banner (author) block should go away and replaced by a hero. WDYT?

@mhaack : But let's not throw away the Title Banner block, as we still need it for Topic and Tag pages

@pahupe @mhaack why not use a Hero? I don't think "Title Banner" is something that exists, it was more of a placeholder before we knew better

@mhaack
Copy link
Collaborator

mhaack commented Mar 12, 2024

@mhaack : But let's not throw away the Title Banner block, as we still need it for Topic and Tag pages

@pahupe why?

@pahupe
Copy link
Contributor Author

pahupe commented Mar 13, 2024

@mhaack : But let's not throw away the Title Banner block, as we still need it for Topic and Tag pages

@pahupe why?

@mhaack : I suggest to do that in a separate ticket: Let's complete this PR so we have the layout for the current content fixed, and then let's address the content replacement (title banner => hero) separately. What do you think?

@benpeter
Copy link
Collaborator

@mhaack : But let's not throw away the Title Banner block, as we still need it for Topic and Tag pages

@pahupe why?

@mhaack : I suggest to do that in a separate ticket: Let's complete this PR so we have the layout for the current content fixed, and then let's address the content replacement (title banner => hero) separately. What do you think?

@pahupe why? it's just 2 or 3 word docs, right?

@pahupe
Copy link
Contributor Author

pahupe commented Mar 13, 2024

@mhaack : But let's not throw away the Title Banner block, as we still need it for Topic and Tag pages

@pahupe why?

@mhaack : I suggest to do that in a separate ticket: Let's complete this PR so we have the layout for the current content fixed, and then let's address the content replacement (title banner => hero) separately. What do you think?

@pahupe why? it's just 2 or 3 word docs, right?

@mhaack , @benpeter :

  • Happy to replace the title banner / column block on all "full author" pages like Julia White, Scott Russell, Christian Klein, etc. with a Hero. Some of this has actually already been done, see e.g., https://hero-2--hlx-test--urfuwo.hlx.live/author/juliawhite. I would assume more content rework for the other "full author" pages can be done if needed, but independently of this PR, as it is content work
  • We can also see if we use a Hero on the dynamically generated author pages. I suggest to do that content rework separately from this PR. Also, we need this caching issue addressed: https://adobe-dx-support.slack.com/archives/C06H8UNQ3RS/p1710229078307869
  • For tags and topics dynamic pages, we currently use the title banner custom logic to extract a displayable title, see code in title-banner.js. I don't think this can be achieved simply by using placeholders, so some of that logic would end up in the hero component. We could do that, but I propose to do that in a separate ticket
  • In general: we now (6 days before Mid-March) have a solution where we can showcase heros on author pages (https://hero-2--hlx-test--urfuwo.hlx.live/author/juliawhite) and topic pages (https://250-non-curated-author-pages--hlx-test--urfuwo.hlx.live/topics/cloud). I would put the title-banner => hero replacement task into a separate ticket, and if it fits before Mid-March, let's tackle it, or else later

@aem-code-sync aem-code-sync bot temporarily deployed to 250-non-curated-author-pages March 13, 2024 11:51 Inactive
@mhaack
Copy link
Collaborator

mhaack commented Mar 13, 2024

@pahupe do you want to rebase this on main first. Also please double check the LHS check, the dynamic pages look like constantly not getting 100?

@aem-code-sync aem-code-sync bot temporarily deployed to 250-non-curated-author-pages March 13, 2024 16:59 Inactive
@pahupe
Copy link
Contributor Author

pahupe commented Mar 13, 2024

@pahupe do you want to rebase this on main first. Also please double check the LHS check, the dynamic pages look like constantly not getting 100?

@mhaack :

@aem-code-sync aem-code-sync bot temporarily deployed to 250-non-curated-author-pages March 14, 2024 07:27 Inactive
Copy link
Collaborator

@saurabh-khare saurabh-khare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pahupe Do we have any impediments that prevent merging this PR?

@mhaack
Copy link
Collaborator

mhaack commented Mar 14, 2024

I think, after conflicts are merged this is good to merge.

@pahupe pahupe merged commit d9ced47 into main Mar 14, 2024
2 checks passed
@pahupe pahupe deleted the 250-non-curated-author-pages branch March 14, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-curated L2 Topic and Tag Pages Author, topic and tag pages
4 participants