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.

Sign up for GitHub

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

Jump to bottom

SDSS-592: Add sidebar component #172

Merged
merged 15 commits into from
Jul 19, 2023
Merged

SDSS-592: Add sidebar component #172

merged 15 commits into from
Jul 19, 2023

Conversation

jenbreese
Copy link
Contributor

@jenbreese jenbreese commented Jul 3, 2023 •
edited
Loading

READY FOR REVIEW

Summary

  • Added a sidebar component
  • Removed the 2 and 3 column layout options.
  • Updated the name to Editorial Sidebar
  • Changed organization of scss files.

Review By (Date)

  • Soon

Criticality

  • normal

Review Tasks

Setup tasks and/or behavior to test

  1. Check out this branch
  2. Navigate to a page and add a sidebar component.
  3. Verify it adds one and looks like the design.
  4. This branch is based off the SDSS-591--add-callout-component because of shared reasons.

Front End Validation

  • Is the markup using the appropriate semantic tags and passes HTML validation?
  • Cross-browser testing has been performed?
  • Automated accessibility scans performed?
  • Manual accessibility tests performed?
  • Design is approved by @ user?

Backend / Functional Validation

Code

  • Are the naming conventions following our standards?
  • Does the code have sufficient inline comments?
  • Is there anything in this code that would be hidden or hard to discover through the UI?
  • Are there any code smells?
  • Are tests provided? eg (unit, behat, or codeception)

Code security

General

  • Is there anything included in this PR that is not related to the problem it is trying to solve?
  • Is the approach to the problem appropriate?

Affected Projects or Products

  • Does this PR impact any particular projects, products, or modules?

Associated Issues and/or People

- SDSS-592

Resources

@jenbreese jenbreese changed the base branch from 2.x to feature/newsroom-layout-paragraphs July 3, 2023 19:10
@jenbreese jenbreese changed the base branch from feature/newsroom-layout-paragraphs to SDSS-591--add-callout-component July 3, 2023 19:14
@jenbreese jenbreese requested a review from joegl July 5, 2023 20:09
@joegl joegl changed the title Sdss 592 sidebar component v2 SDSS-592: Add sidebar component Jul 7, 2023
joegl
joegl requested changes Jul 7, 2023
View reviewed changes
Copy link
Collaborator

@joegl joegl left a comment

Choose a reason for hiding this comment

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

I didn't pull this one down and look at the display yet.

I also asked in Slack if we could change the name of the component. I don't think Sidebar really fits (and it's creating a lot of confusion for me).

...dss/sdss_profile/config/sync/core.entity_form_display.paragraph.newsroom_sidebar.default.yml Outdated
bundle: newsroom_sidebar
mode: default
content:
su_sidebar_blue_bg:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Like the other PR, we should stay away from using specific color names in the id/machine name, in case colors or options need to change.

We may also want to explore a select field like the other PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

docroot/profiles/sdss/sdss_profile/config/sync/paragraphs.paragraphs_type.stanford_layout.yml Show resolved Hide resolved
.../profiles/sdss/sdss_profile/modules/sdss_layout_paragraphs/layouts/two_column/two-column.css Outdated
@@ -11,7 +11,7 @@
}
}

@media screen and (min-width: 992px) {
@media screen and (min-width: 1199px) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this change for? Especially if we're doing only 1 column right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Undid this change.

docroot/profiles/sdss/sdss_profile/themes/sdss_subtheme/src/scss/components/index.scss Outdated
@@ -33,6 +33,7 @@
@import 'main-navigation/index.scss';
@import 'news/index.scss';
@import 'newsroom-callout/index.scss';
@import 'newsroom-sidebar/index.scss';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Like the callout PR, should this be a component just called "sidebar"? And not "newsroom-sidebar"? It'd probably be easier to discuss this outside of the PR

...es/sdss/sdss_profile/themes/sdss_subtheme/src/scss/components/newsroom-sidebar/_sidebar.scss Outdated

&.blue_bg {
@include sdss-light-blue--background;
// background-color: orange;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// background-color: orange;

...es/sdss/sdss_profile/themes/sdss_subtheme/src/scss/components/newsroom-sidebar/_sidebar.scss Outdated
Comment on lines 26 to 29
.su-sidebar-blue-bg,
.su-sidebar-white-bg {
display: none;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should be hidden within the template/manage display and not with CSS.

Base automatically changed from SDSS-591--add-callout-component to feature/newsroom-layout-paragraphs July 19, 2023 20:07
@joegl joegl changed the base branch from feature/newsroom-layout-paragraphs to 2.x July 19, 2023 20:21
@joegl joegl changed the base branch from 2.x to feature/newsroom-layout-paragraphs July 19, 2023 20:21
@joegl joegl merged commit d69e84c into feature/newsroom-layout-paragraphs Jul 19, 2023
4 checks passed
@joegl joegl deleted the SDSS-592--sidebar-component-v2 branch July 19, 2023 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joegl joegl joegl approved these changes

Assignees

@joegl joegl

Labels
size/s
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jenbreese @joegl