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

Breaking: Convert indicator to jsx, add outer container (fixes 230) #231

Merged
merged 13 commits into from
Jun 24, 2024

Conversation

oliverfoster
Copy link
Member

@oliverfoster oliverfoster commented Jun 12, 2024

fixes #230

New

  • Added .pagelevelprogress__indicator-outer as a style neutral container
  • Used css variable --adapt-pagelevelprogress-percentage on each container to allow use in other styling

Breaking

  • Switch from hbs to jsx templates

@oliverfoster oliverfoster self-assigned this Jun 12, 2024
Base automatically changed from issue/224 to master June 12, 2024 09:05
Copy link
Contributor

Choose a reason for hiding this comment

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

👀

@oliverfoster
Copy link
Member Author

oliverfoster commented Jun 12, 2024

js-indicator-aria-label was there so that the view could use jquery to update the dom after it had been initially rendered by handlebars.

Instead it now updates a css variable on the outer and then uses that variable in the stylesheet to update the width. It means that it's easier for any other template / styling to repurpose the css variable, say if you wanted to do a column chart instead of a bar chart. Rather than the view targeting the bar element directly, css cascades the value through the descendants and it can be picked up by any descendant rule.

setPercentageComplete() {
const percentage = this.calculatePercentage();
this.model.set('percentageComplete', percentage);
this.$el.css({
'--adapt-pagelevelprogress-percentage': percentage + '%'
});
return percentage;
}

&__indicator-bar {
display: block;
height: inherit;
min-width: 15%;
background-color: @black;
width: var(--adapt-pagelevelprogress-percentage);
}

It can be done like this, or I can use the value in the template and set the style width value on the element.

Copy link
Contributor

@joe-allen-89 joe-allen-89 left a comment

Choose a reason for hiding this comment

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

Looking good, one change and one query.

templates/pageLevelProgressIndicator.jsx Outdated Show resolved Hide resolved
js/PageLevelProgressIndicatorView.js Show resolved Hide resolved
Copy link
Contributor

@joe-allen-89 joe-allen-89 left a comment

Choose a reason for hiding this comment

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

👍 All working as expected.

Copy link
Contributor

@guywillis guywillis left a comment

Choose a reason for hiding this comment

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

👀

Copy link
Contributor

@kirsty-hames kirsty-hames left a comment

Choose a reason for hiding this comment

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

Works as expected thanks 👍

@oliverfoster oliverfoster merged commit 26088fb into master Jun 24, 2024
@oliverfoster oliverfoster deleted the issue/230 branch June 24, 2024 09:29
github-actions bot pushed a commit that referenced this pull request Jun 24, 2024
# [8.0.0](v7.9.0...v8.0.0) (2024-06-24)

### Breaking

* Convert indicator to jsx, add outer container (fixes 230) (#231) ([26088fb](26088fb)), closes [#231](#231)
Copy link

🎉 This PR is included in version 8.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

Support more easy replacement of bar graphs
5 participants