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

fix(banner): fix issue with classes not working if banner type provided #887

Conversation

AlexBramhill
Copy link
Contributor

@AlexBramhill AlexBramhill commented Oct 29, 2024

Identify the bug

Link to the issue describing the bug that you're fixing.

Description of the change

  • Existing implementation appears to have an else and endIf block in a place that disables classes if type is set:
// Existing implementation (spacing added for readability)
<div class=
  "moj-banner
  {% if params.type == 'success' %} moj-banner--success
  {% elif params.type == 'warning' %} moj-banner--warning
  {% else %}           // This else and endIf below cause the bug
  {{- ' ' + params.classes if params.classes}}
  {% endif %}"
  ...
>
  • Updated and refactored file to address this bug, and increase readability following pattern of declaring classes logic above component as per recent button-menu commit to main

Alternative designs

  • I have a commit on this branch (hash 5742f092b7cc670d6ab453459f03c1252964d07e) that does not do the refactor, but addresses the bug

Possible drawbacks

If this is an intended business logic, then this change is not needed.

Verification process

Manual testing:

  • check can set type and component reflects this
  • check can set classes and component reflects this
  • check can set type and classes and component reflects this
  • check if type and classes contradict, type takes precedence

Release notes

Fixed #886 where if a banner type is present, banner classes could not be set

@AlexBramhill AlexBramhill requested a review from a team as a code owner October 29, 2024 16:00
@AlexBramhill AlexBramhill changed the title Fix/banner/fix issue with classes not working if banner type provided fix(banner): fix issue with classes not working if banner type provided Oct 29, 2024
Copy link
Contributor

@chrispymm chrispymm left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I'm pretty sure this is a bug and not intended behaviour.

Changes look good and the refactor is nice too.

Spotted one issue, where I think you're missing set. If you can fix that up, then I can get this merged.

src/moj/components/banner/template.njk Outdated Show resolved Hide resolved
src/moj/components/banner/template.njk Outdated Show resolved Hide resolved
@chrispymm chrispymm self-requested a review October 30, 2024 12:09
@chrispymm chrispymm merged commit fa418d4 into ministryofjustice:main Nov 5, 2024
18 checks passed
gregtyler pushed a commit that referenced this pull request Nov 5, 2024
## [3.0.2](v3.0.1...v3.0.2) (2024-11-05)

### Bug Fixes

* **banner:** apply custom classes when type is provided ([#887](#887)) ([fa418d4](fa418d4))
* **button menu:** fix alignment within moj-button-group without js ([#904](#904)) ([9e6d87d](9e6d87d))
@gregtyler
Copy link
Contributor

🎉 This PR is included in version 3.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Banner classes parameter does not work when type parameter is set
3 participants