From 096cf18b1047e5658af56d61fb14e536beff6ccb Mon Sep 17 00:00:00 2001 From: ludwiczakpawel Date: Thu, 30 Sep 2021 19:08:06 +0200 Subject: [PATCH] Article layout (#14753) * style updates * author and tools * overflow hidden for avatars because geeeez * font smoothing * reverting little change * specs * preview * css fix * avatar fix' * No longer eager load user profile when showing articles. articles show view no longer needs user profile, as it no longer renders the user profile preview. * tags * tags * tags reverts * typo * dropping initializer for user cards previews on article page * spec * remove spec * revert * Update app/decorators/article_decorator.rb Co-authored-by: Michael Kohl * Update spec/requests/stories_show_spec.rb Co-authored-by: Michael Kohl * yoloing outline change on buttons because i couldn't see any visual difference Co-authored-by: Dan Uber Co-authored-by: Michael Kohl --- .../initializers/initializeBaseUserData.js | 8 +- app/assets/stylesheets/base/typography.scss | 3 + .../stylesheets/components/avatars.scss | 2 + .../stylesheets/components/buttons.scss | 1 - .../stylesheets/components/stories.scss | 5 +- app/assets/stylesheets/views/article.scss | 10 + app/controllers/stories_controller.rb | 2 +- app/decorators/article_decorator.rb | 2 +- .../article-form/components/Preview.jsx | 8 +- app/javascript/packs/articlePage.jsx | 43 +-- app/views/articles/_sticky_nav.html.erb | 1 - app/views/articles/show.html.erb | 103 +++--- .../articleFlows/previewProfile.spec.js | 314 ------------------ .../publishingFlows/postOptions.spec.js | 2 +- spec/requests/stories_show_spec.rb | 2 +- spec/system/user_uses_the_editor_spec.rb | 3 +- 16 files changed, 80 insertions(+), 429 deletions(-) delete mode 100644 cypress/integration/seededFlows/articleFlows/previewProfile.spec.js diff --git a/app/assets/javascripts/initializers/initializeBaseUserData.js b/app/assets/javascripts/initializers/initializeBaseUserData.js index 0bb939fe3..843c0ed66 100644 --- a/app/assets/javascripts/initializers/initializeBaseUserData.js +++ b/app/assets/javascripts/initializers/initializeBaseUserData.js @@ -17,7 +17,7 @@ function addRelevantButtonsToArticle(user) { if (parseInt(articleContainer.dataset.authorId, 10) === user.id) { actions.push( - `Edit`, + `Edit`, ); let clickToEditButton = document.getElementById('author-click-to-edit'); @@ -27,12 +27,12 @@ function addRelevantButtonsToArticle(user) { if (published === true) { actions.push( - `Manage`, + `Manage`, ); } actions.push( - `Stats`, + `Stats`, ); } @@ -42,7 +42,7 @@ function addRelevantButtonsToArticle(user) { // if there's already a pinned post different from the current one if (user.admin) { actions.push( - `Admin`, + `Admin`, ); } diff --git a/app/assets/stylesheets/base/typography.scss b/app/assets/stylesheets/base/typography.scss index d0221a063..40fb93b6f 100644 --- a/app/assets/stylesheets/base/typography.scss +++ b/app/assets/stylesheets/base/typography.scss @@ -43,6 +43,9 @@ // Content fonts body { + -webkit-font-smoothing: antialiased; + -moz-osx-font-smoothing: grayscale; + &.serif-article-body { --content-font-family: var(--ff-serif); } diff --git a/app/assets/stylesheets/components/avatars.scss b/app/assets/stylesheets/components/avatars.scss index 1713eac68..7471a8bab 100644 --- a/app/assets/stylesheets/components/avatars.scss +++ b/app/assets/stylesheets/components/avatars.scss @@ -19,6 +19,8 @@ background-color: $background; width: var(--su-6); height: var(--su-6); + overflow: hidden; + vertical-align: middle; &__image { border-radius: $radius; diff --git a/app/assets/stylesheets/components/buttons.scss b/app/assets/stylesheets/components/buttons.scss index 88308c2f1..549c962b7 100644 --- a/app/assets/stylesheets/components/buttons.scss +++ b/app/assets/stylesheets/components/buttons.scss @@ -42,7 +42,6 @@ padding: calc(var(--vertical-padding) - var(--border-width)) calc(var(--horizontal-padding) - var(--border-width)); border-radius: var(--radius); - outline: none; font-family: inherit; font-size: var(--font-size); line-height: var(--su-6); // intentionally not using $lh-* variable. diff --git a/app/assets/stylesheets/components/stories.scss b/app/assets/stylesheets/components/stories.scss index e343bb520..5868baa71 100644 --- a/app/assets/stylesheets/components/stories.scss +++ b/app/assets/stylesheets/components/stories.scss @@ -77,7 +77,10 @@ padding-bottom: 42%; background-size: cover; background-position: center center; - border-radius: var(--radius) var(--radius) 0 0; + + @media (min-width: $breakpoint-s) { + border-radius: var(--radius) var(--radius) 0 0; + } } &__body { diff --git a/app/assets/stylesheets/views/article.scss b/app/assets/stylesheets/views/article.scss index 5833eb475..89006ece4 100644 --- a/app/assets/stylesheets/views/article.scss +++ b/app/assets/stylesheets/views/article.scss @@ -48,6 +48,16 @@ } } + &__actions { + &:empty { + display: none; + } + background: var(--accent-warning-a10); + border: 1px solid var(--accent-warning-a10); + border-radius: var(--radius); + padding: var(--su-1); + } + &__subheader { font-size: var(--fs-base); color: var(--base-60); diff --git a/app/controllers/stories_controller.rb b/app/controllers/stories_controller.rb index 0f783dec2..01b1c1e1b 100644 --- a/app/controllers/stories_controller.rb +++ b/app/controllers/stories_controller.rb @@ -34,7 +34,7 @@ class StoriesController < ApplicationController def show @story_show = true path = "/#{params[:username].downcase}/#{params[:slug]}" - if (@article = Article.includes(user: :profile).find_by(path: path)&.decorate) + if (@article = Article.includes(:user).find_by(path: path)&.decorate) handle_article_show elsif (@article = Article.find_by(slug: params[:slug])&.decorate) handle_possible_redirect diff --git a/app/decorators/article_decorator.rb b/app/decorators/article_decorator.rb index 7e0dc51e8..acf0254bf 100644 --- a/app/decorators/article_decorator.rb +++ b/app/decorators/article_decorator.rb @@ -99,7 +99,7 @@ class ArticleDecorator < ApplicationDecorator def co_author_name_and_path co_authors.map do |user| - "#{user.name}" + %(#{user.name}) end.to_sentence end diff --git a/app/javascript/article-form/components/Preview.jsx b/app/javascript/article-form/components/Preview.jsx index 58a99bb09..074c23a65 100644 --- a/app/javascript/article-form/components/Preview.jsx +++ b/app/javascript/article-form/components/Preview.jsx @@ -16,7 +16,7 @@ function titleArea({ tags = tagArray.map((tag) => { return ( tag.length > 0 && ( - + # {tag} @@ -26,7 +26,7 @@ function titleArea({ } // The v2 editor stores its cover image in articleState.mainImage, while the v1 editor - // stores it as previewRespose.cover_image. When previewing, we handle both by + // stores it as previewResponse.cover_image. When previewing, we handle both by // defaulting to setting the cover image to the mainImage on the article (v2), // and only using the cover image from the previewResponse if it exists (v1). let coverImage = articleState.mainImage || ''; @@ -60,11 +60,11 @@ function titleArea({ {!errors && markdownLintErrors?.length > 0 && ( )} -

+

{previewTitle}

-
{tags}
+
{tags}
); diff --git a/app/javascript/packs/articlePage.jsx b/app/javascript/packs/articlePage.jsx index 2100096f4..0c12a8f6b 100644 --- a/app/javascript/packs/articlePage.jsx +++ b/app/javascript/packs/articlePage.jsx @@ -1,12 +1,8 @@ import { h, render } from 'preact'; import { Snackbar, addSnackbarItem } from '../Snackbar'; import { addFullScreenModeControl } from '../utilities/codeFullscreenModeSwitcher'; +import { initializeDropdown } from '../utilities/dropdownUtils'; import { embedGists } from '../utilities/gist'; -import { - initializeDropdown, - getDropdownRepositionListener, -} from '../utilities/dropdownUtils'; -import { getInstantClick } from '../topNavigation/utilities'; /* global Runtime */ @@ -123,42 +119,5 @@ getCsrfToken().then(async () => { } }); -// Initialize the profile preview functionality -const profilePreviewTrigger = document.getElementById( - 'profile-preview-trigger', -); - -const dropdownContent = document.getElementById('profile-preview-content'); - -if (profilePreviewTrigger?.dataset.initialized !== 'true') { - initializeDropdown({ - triggerElementId: 'profile-preview-trigger', - dropdownContentId: 'profile-preview-content', - onOpen: () => { - dropdownContent?.classList.add('showing'); - }, - onClose: () => { - dropdownContent?.classList.remove('showing'); - }, - }); - - profilePreviewTrigger.dataset.initialized = 'true'; -} - const targetNode = document.querySelector('#comments'); targetNode && embedGists(targetNode); - -// Preview card dropdowns reposition on scroll -const dropdownRepositionListener = getDropdownRepositionListener(); - -document.addEventListener('scroll', dropdownRepositionListener); - -getInstantClick().then((ic) => { - ic.on('change', () => { - document.removeEventListener('scroll', dropdownRepositionListener); - }); -}); - -window.addEventListener('beforeunload', () => { - document.removeEventListener('scroll', dropdownRepositionListener); -}); diff --git a/app/views/articles/_sticky_nav.html.erb b/app/views/articles/_sticky_nav.html.erb index 4b4684be5..35561ca36 100644 --- a/app/views/articles/_sticky_nav.html.erb +++ b/app/views/articles/_sticky_nav.html.erb @@ -1,6 +1,5 @@ <% @actor = @article.organization || @article.user %>
-
<%= render "shared/profile_card_content", actor: @actor, context: "sidebar" %>
diff --git a/app/views/articles/show.html.erb b/app/views/articles/show.html.erb index 8d404a9dc..9bd05e4da 100644 --- a/app/views/articles/show.html.erb +++ b/app/views/articles/show.html.erb @@ -97,15 +97,51 @@ <% end %>
- <% if @organization %> - - - <%= @organization.name %> - - <% end %> -

+
+
+
+
+ <% if @organization %> + <%= @organization.name %> profile image + + <%= @user.name %> + + <% else %> + <%= @user.name %> + <% end %> +
+
+ <%= @user.name %> + <% if @organization %> + for <%= @organization.name %> + <% end %> +

+ <% if @article.published_timestamp.present? %> + Posted on + <%= local_date(@article.published_timestamp, show_year: @article.displayable_published_at.year != Time.current.year) %> + <% end %> + <% if @article.co_author_ids.present? %> + with <%= @article.co_author_name_and_path.html_safe %> + <% end %> + + <% if should_show_updated_on?(@article) %> + • Updated on <%= local_date(@article.edited_at, show_year: @article.edited_at.year != Time.current.year) %> + <% end %> + + <% if should_show_crossposted_on?(@article) %> + • Originally published at + <%= get_host_without_www(@article.canonical_url || @article.feed_source_url) %> + <% if @article.crossposted_at %> + on + <%= local_date(@article.originally_published_at || @article.published_at, show_year: (@article.originally_published_at || @article.published_at).year != Time.current.year) %> + <% end %> + <% end %> +

+
+
+
+ +

<% if @article.search_optimized_title_preamble.present? && !user_signed_in? %> <%= @article.search_optimized_title_preamble %> <% end %> @@ -113,57 +149,12 @@

<% cache("main-article-tags-#{@article.cached_tag_list}", expires_in: 30.hours) do %> -
+
<% @article.cached_tag_list_array.each do |tag| %> - <% tag_hash = tag_colors(tag) %> - <% styles = "" %> - <% if tag_hash[:background] %> - <% styles = "background-color:#{tag_hash[:background]};" %> - <% end %> - <% if tag_hash[:color] %> - <% styles = "#{styles}color:#{tag_hash[:color]};" %> - <% end %> - #<%= tag %> + #<%= tag %> <% end %>
<% end %> - -
- - - <%= @user.name %> - -
- - <%= render "/shared/profile_preview_card", actor: @user, id: "profile-preview-content" %> -
- - - <% if @article.published_timestamp.present? %> - <%= local_date(@article.published_timestamp, show_year: @article.displayable_published_at.year != Time.current.year) %> - <% end %> - <% if @article.co_author_ids.present? %> - with <%= @article.co_author_name_and_path.html_safe %> - <% end %> - - <% if should_show_crossposted_on?(@article) %> - - Originally published at - <%= get_host_without_www(@article.canonical_url || @article.feed_source_url) %> - <% if @article.crossposted_at %> - on - <%= local_date(@article.originally_published_at || @article.published_at, show_year: (@article.originally_published_at || @article.published_at).year != Time.current.year) %> - <% end %> - - <% end %> - <% if should_show_updated_on?(@article) %> - ・Updated on <%= local_date(@article.edited_at, show_year: @article.edited_at.year != Time.current.year) %> - <% end %> - - ・<%= @article.reading_time < 1 ? 1 : @article.reading_time %> min read - - -
diff --git a/cypress/integration/seededFlows/articleFlows/previewProfile.spec.js b/cypress/integration/seededFlows/articleFlows/previewProfile.spec.js deleted file mode 100644 index 67fa5aaa0..000000000 --- a/cypress/integration/seededFlows/articleFlows/previewProfile.spec.js +++ /dev/null @@ -1,314 +0,0 @@ -describe('Preview user profile from article page', () => { - describe('mobile screens', () => { - beforeEach(() => { - cy.testSetup(); - cy.fixture('users/articleEditorV1User.json').as('user'); - - cy.viewport('iphone-7'); - - cy.get('@user').then((user) => { - cy.loginAndVisit(user, '/').then(() => { - cy.findAllByRole('link', { name: 'Test article' }) - .first() - .click({ force: true }); - - // Wait for page to load - cy.findByRole('button', { name: 'Share post options' }); - }); - }); - }); - - it('should not show a preview profile details button', () => { - cy.findByRole('button', { name: 'Admin McAdmin profile details' }) - .as('previewCardTrigger') - .should('not.exist'); - - // Check the user profile link is shown instead (there are also some in the user details card at the bottom) - cy.findAllByRole('link', { name: 'Admin McAdmin' }).should( - 'have.length', - 3, - ); - }); - }); - - describe("Preview profile on another user's article", () => { - beforeEach(() => { - cy.testSetup(); - cy.fixture('users/articleEditorV1User.json').as('user'); - - cy.get('@user').then((user) => { - cy.loginAndVisit(user, '/admin_mcadmin/test-article-slug'); - - // Wait for the page to load - cy.findByRole('button', { name: 'Share post options' }); - cy.get('[data-follow-clicks-initialized]'); - }); - }); - - it('should show a toggleable preview card for author byline', () => { - cy.findAllByRole('button', { name: 'Admin McAdmin profile details' }) - .first() - .as('previewCardTrigger'); - - // Initializes as unexpanded - cy.get('@previewCardTrigger').should( - 'have.attr', - 'aria-expanded', - 'false', - ); - - // Check the content opens and manages focus - cy.get('@previewCardTrigger').click(); - cy.get('@previewCardTrigger').should( - 'have.attr', - 'aria-expanded', - 'true', - ); - - cy.findAllByTestId('profile-preview-card') - .first() - .within(() => { - cy.findByRole('link', { - name: 'Admin McAdmin', - }).should('have.focus'); - - // Check all the expected user data sections are present - cy.findByText('Admin user summary'); - cy.findByText('Software developer at Company'); - cy.findByText('Edinburgh'); - cy.findByText('University of Life'); - - cy.findByRole('button', { name: 'Follow user: Admin McAdmin' }).as( - 'followUserButton', - ); - - cy.get('@followUserButton').should( - 'have.attr', - 'aria-pressed', - 'false', - ); - cy.get('@followUserButton').click(); - - cy.get('@followUserButton').should( - 'have.attr', - 'aria-pressed', - 'true', - ); - }); - - // Check we can close the preview dropdown - cy.get('@previewCardTrigger').click(); - cy.findAllByTestId('profile-preview-card') - .first() - .should('not.be.visible'); - cy.get('@previewCardTrigger').should( - 'have.attr', - 'aria-expanded', - 'false', - ); - }); - - it('should show a preview card for comment name', () => { - cy.findByTestId('comments-container').within(() => { - cy.findByRole('button', { name: 'Admin McAdmin profile details' }).as( - 'previewCardTrigger', - ); - - cy.get('[data-initialized]'); - cy.get('@previewCardTrigger').click(); - - cy.findByTestId('profile-preview-card').within(() => { - cy.findByRole('link', { - name: 'Admin McAdmin', - }).should('have.focus'); - - // Check all the expected user data sections are present - cy.findByText('Admin user summary'); - cy.findByText('Software developer at Company'); - cy.findByText('Edinburgh'); - cy.findByText('University of Life'); - - cy.findByRole('button', { - name: 'Follow user: Admin McAdmin', - }).as('userFollowButton'); - - cy.get('@userFollowButton').click(); - cy.get('@userFollowButton').should( - 'have.attr', - 'aria-pressed', - 'true', - ); - }); - }); - }); - - it('should update any other matching follow buttons when follow CTA is clicked', () => { - cy.get('[data-initialized]'); - // Click the follow button in the author byline preview - cy.findAllByRole('button', { name: 'Admin McAdmin profile details' }) - .first() - .as('bylinePreviewButton'); - cy.get('@bylinePreviewButton').click(); - - cy.findAllByTestId('profile-preview-card') - .first() - .within(() => { - cy.findByRole('button', { - name: 'Follow user: Admin McAdmin', - }).as('userFollowButton'); - - cy.get('@userFollowButton').click(); - cy.get('@userFollowButton').should( - 'have.attr', - 'aria-pressed', - 'true', - ); - }); - - // Close the preview card so the next preview button can be clicked - cy.get('@bylinePreviewButton').click(); - - // Check the follow button in the comment author preview card has updated - cy.findAllByRole('button', { name: 'Admin McAdmin profile details' }) - .last() - .click(); - - cy.findAllByTestId('profile-preview-card') - .last() - .findByRole('button', { name: 'Follow user: Admin McAdmin' }) - .should('have.attr', 'aria-pressed', 'true'); - }); - - it('should detach listeners on preview card close', () => { - cy.findAllByRole('button', { name: 'Admin McAdmin profile details' }) - .first() - .as('previewCardTrigger'); - - // Make sure button has initialized - cy.get('[data-initialized]').should('exist'); - - // Open the preview - cy.get('@previewCardTrigger').click(); - cy.get('@previewCardTrigger').should( - 'have.attr', - 'aria-expanded', - 'true', - ); - - // Close by pressing Escape - cy.get('body').type('{esc}'); - cy.get('@previewCardTrigger').should( - 'have.attr', - 'aria-expanded', - 'false', - ); - cy.get('@previewCardTrigger').should('have.focus'); - - // Focus another item on the page - cy.findByRole('button', { name: 'Share post options' }).focus(); - - // Press Escape again and check the focus isn't reverted back to the dropdown trigger - cy.get('body').type('{esc}'); - cy.get('@previewCardTrigger').should('not.have.focus'); - - // Click on a non-interactive element and check the focus isn't reverted back to the dropdown trigger - cy.findByRole('heading', { name: 'Test article' }).click(); - cy.get('@previewCardTrigger').should('not.have.focus'); - }); - }); - - describe("Preview profile on user's own article", () => { - beforeEach(() => { - cy.testSetup(); - cy.fixture('users/articleEditorV1User.json').as('user'); - - cy.get('@user').then((user) => { - cy.loginUser(user).then(() => { - cy.createArticle({ - title: 'Test Article', - tags: ['beginner', 'ruby', 'go'], - content: `This is a test article's contents.`, - published: true, - }).then((response) => { - cy.visitAndWaitForUserSideEffects(response.body.current_state_path); - }); - }); - }); - }); - - it('should show a preview card with Edit profile CTA', () => { - cy.findByRole('button', { - name: 'Article Editor v1 User profile details', - }).as('previewCardTrigger'); - - // Initializes as unexpanded - cy.get('@previewCardTrigger').should( - 'have.attr', - 'aria-expanded', - 'false', - ); - - // Check the content opens and manages focus - cy.get('@previewCardTrigger').click(); - cy.get('@previewCardTrigger').should( - 'have.attr', - 'aria-expanded', - 'true', - ); - - cy.findByTestId('profile-preview-card').within(() => { - cy.findByRole('link', { - name: 'Article Editor v1 User', - }).should('have.focus'); - - // Check Follow option is replaced with Edit profile - cy.findByRole('button', { name: 'Edit profile' }).click(); - }); - - // Check Edit profile directs to correct page - cy.url().should('contain', '/settings'); - cy.findByRole('heading', { - name: 'Settings for @article_editor_v1_user', - }); - }); - }); - - describe('Preview author profile on an organization article', () => { - beforeEach(() => { - cy.testSetup(); - cy.fixture('users/articleEditorV1User.json').as('user'); - - cy.get('@user').then((user) => { - cy.loginUser(user).then(() => { - cy.visit('/'); - cy.findAllByRole('link', { name: 'Organization test article' }) - .first() - .click({ force: true }); - // Make sure we have arrived on the article page - cy.findByRole('button', { name: 'Share post options' }); - }); - }); - }); - - it('Should show author details in the preview card', () => { - cy.findByRole('button', { name: 'Admin McAdmin profile details' }).as( - 'profileDetailsButton', - ); - // Make sure the button is initialized before interacting - cy.get('[data-initialized]'); - cy.get('@profileDetailsButton').click(); - cy.findByTestId('profile-preview-card').within(() => { - // Check user fields are present - cy.findByRole('link', { - name: 'Admin McAdmin', - }).should('have.focus'); - - cy.findByRole('button', { name: 'Follow user: Admin McAdmin' }); - cy.findByText('Admin user summary'); - cy.findByText('Software developer at Company'); - cy.findByText('Edinburgh'); - cy.findByText('University of Life'); - }); - }); - }); -}); diff --git a/cypress/integration/seededFlows/publishingFlows/postOptions.spec.js b/cypress/integration/seededFlows/publishingFlows/postOptions.spec.js index e4127b8a7..07fa44d7b 100644 --- a/cypress/integration/seededFlows/publishingFlows/postOptions.spec.js +++ b/cypress/integration/seededFlows/publishingFlows/postOptions.spec.js @@ -69,7 +69,7 @@ describe('V2 Editor Post options', () => { }); // Verify that the canonical URL is used - cy.findByText('Originally published at').should('exist'); + cy.findByText(/Originally published at/).should('exist'); cy.findByRole('link', { name: 'exampleurl.com' }); }); diff --git a/spec/requests/stories_show_spec.rb b/spec/requests/stories_show_spec.rb index bfb42d069..5b5ac9577 100644 --- a/spec/requests/stories_show_spec.rb +++ b/spec/requests/stories_show_spec.rb @@ -134,7 +134,7 @@ RSpec.describe "StoriesShow", type: :request do user2 = create(:user) article.update(co_author_ids: [user2.id]) get article.path - expect(response.body).to include "with " + expect(response.body).to include %(with ) end it "renders articles of long length without breaking" do diff --git a/spec/system/user_uses_the_editor_spec.rb b/spec/system/user_uses_the_editor_spec.rb index ec9cbc1a7..aa6362959 100644 --- a/spec/system/user_uses_the_editor_spec.rb +++ b/spec/system/user_uses_the_editor_spec.rb @@ -105,8 +105,7 @@ RSpec.describe "Using the editor", type: :system do expect(page).to have_xpath("//div[@class='crayons-article__header__meta']//h1") expect(page).to have_text("Hello") - expect(page).not_to have_text("") - expect(page).to have_link("#what", href: "/t/what") + expect(page).to have_link("what", href: "/t/what") end end end