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

Notice refactor #1166

Merged
merged 9 commits into from
Jul 8, 2020
Merged

Notice refactor #1166

merged 9 commits into from
Jul 8, 2020

Conversation

ianmcburnie
Copy link
Contributor

@ianmcburnie ianmcburnie commented Jul 6, 2020

DON'T SQUASH.

Issue #1001

Reworks the structure to incorporate the visible text into the heading structure, instead of around the icon. While I did like the idea of the region beginning with a heading (in the same way I wish item tiles could start with a heading instead of an image), it just started to feel a bit weird not having the visible text as the heading. Hence these changes.

Before:

<section class="page-notice page-notice--confirmation" role="region" aria-labelledby="confirmation-status">
    <h2 class="page-notice__status" id="confirmation-status">
        <svg class="icon icon--confirmation-filled" focusable="false" height="24" width="24" aria-label="Success">
            <use xlink:href="#icon-confirmation-filled"></use>
        </svg>
    </h2>
    <div class="page-notice__content">
        <p><strong>Congrats!</strong> You just listed <a href="#">"Spam and Eggs From the Cow's Perspective</a> (paperback)."</p>
    </div>
</section>

After

<section class="page-notice page-notice--confirmation" role="region" aria-label="Confirmation">
    <div class="page-notice__header">
        <svg class="icon icon--confirmation-filled" focusable="false" height="24" width="24" role="img" aria-label="Confirmation">
            <use xlink:href="#icon-confirmation-filled"></use>
        </svg>
    </div>
    <div class="page-notice__main">
        <h2 class="page-notice__title">Title copy goes here</h2>
        <p>Details...</p>
    </div>
</section>

Issue #1150

Normalized all notices and split them apart into separate components.

While there is a little bit of duplication, mainly in terms of the flexbox layout, I find the modules now much easier to reason with compared to the grouped selectors we had before. So it seems like a good tradeoff. We can minimise some duplication with mixins if we like.

Visually page notices are now in a half way state between ds4 and ds6.

We can either

a) leave them as is
b) add color background (like ds6)
c) reinstate the grey border colour and thicker top border using system of variables/properties in the base file (i.e. not like we had before, which created a code split in the ds4 and ds6 files)

I don't consider this as a huge priority right now, and will tackle it either way just before we release.

DS4 Page notice before normalization:

Screen Shot 2020-07-05 at 6 13 39 PM

DS4 Page notice after normalization:

Screen Shot 2020-07-05 at 6 13 22 PM

DS6 Page Notice

Screen Shot 2020-07-05 at 6 13 57 PM

@ianmcburnie ianmcburnie requested a review from agliga July 6, 2020 16:33
@ianmcburnie ianmcburnie self-assigned this Jul 6, 2020
Copy link
Contributor

@agliga agliga left a comment

Choose a reason for hiding this comment

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

I think it looks good. Just one comment.
The only thing I would say maybe we should think about common things as mixins, although I don't see very many things that can be put into a common mixin. Maybe the different attention/confirmations/information sections, but it's probably fine.

@ianmcburnie
Copy link
Contributor Author

Yeah, I think there is definitely the potential to leverage some mixins. Will leave that for a future enhancement.

@ianmcburnie ianmcburnie merged commit 8ca84f5 into 11.0.0 Jul 8, 2020
@ianmcburnie ianmcburnie deleted the notice-refactor branch August 18, 2020 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants