Skip to content
This repository has been archived by the owner on Jun 26, 2018. It is now read-only.

DP-4572 - Re-work Dateline #599

Merged
merged 4 commits into from
Oct 30, 2017
Merged

DP-4572 - Re-work Dateline #599

merged 4 commits into from
Oct 30, 2017

Conversation

nstriedinger
Copy link
Contributor

@nstriedinger nstriedinger commented Sep 18, 2017

Description

I reworked the Dateline for the Drupal side. FYI -- I kept the <p> tag for current PL structure, but we will not be using this pattern on the Drupal end. (On the Drupal end, we're separating everything out and adding the Dateline, and reassembling programmatically.)

Related Issue / Ticket

Steps to Test

  1. Read Ticket
  2. go to ?p=pages-press-release
  3. confirm span and bolded dateline.

Screenshots

screen shot 2017-09-18 at 11 29 29 am

Additional Notes:

Anything else to add?

Impacted Areas in Application

@todo

Today I learned...

Copy link
Contributor

@legostud legostud left a comment

Choose a reason for hiding this comment

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

What was the requirement for adding the extra class to make this bold instead of using the b tag?

Personally, I prefer the Drupal solution over what I created. My solution is a bit brittle so I'd be okay with deleting the ma__press__location-label Markup and CSS in favor of just adding the content within the first paragraph of the RTE block. Having it as part of the RTE block was my original solution, because it was simple and elegant.

text-transform: uppercase;
font-weight: bold;
Copy link
Contributor

Choose a reason for hiding this comment

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

font-weight should be part of a theme CSS file not part of the base CSS file (ie: /06-theme/04-templates/_press.scss file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did I add this correctly?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes that looks good

@jesconstantine
Copy link
Contributor

@legostud I didn't fully understand your original change request - was it just to move the style rule into the theme scss or was there something else that you thought needed to be addressed here?

@nstriedinger
Copy link
Contributor Author

nstriedinger commented Sep 27, 2017

@legostud -- @gleroux02 suggested I use a <span>.

@legostud
Copy link
Contributor

This PR doesn't appear to solve any issues so I'm confused as to what's the goal with making this change?

The &__label was already uppercase and the text was already bold. The only difference I see here is that the — is no longer uppercase, which doesn't matter. This PR seems to be just adding bloat for no reason.

@legostud
Copy link
Contributor

I feel like we're missing something. Maybe there is actually more work that needs to be done in Mayflower to match the approach that Mass Gov is using.

@legostud legostud closed this Sep 28, 2017
@legostud legostud reopened this Sep 28, 2017
@legostud
Copy link
Contributor

legostud commented Sep 28, 2017

After talking with Jes, I better understand the desired outcome here. I would suggest removing all the code for the location-label and just add the content <span class="ma__rich-text__flame">Boston</span> &mdash; to the first paragraph of the Rich Text component in Mayflower. In the _rich-text.scss files we can add the styling needed to make the &__flame selector uppercase and bold similar to what you've already created.

What we're doing is adding a 'flame' option to the Rich Text pattern in Mayflower that you can leverage for your current solution.

@@ -1,4 +1,9 @@
.ma__rich-text {

&__flame {
font-weight: bold;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in the 'theme' file for rich-text. Otherwise this looks really good.

Copy link
Contributor

Choose a reason for hiding this comment

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

the font-weight: bold that is

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants