Skip to content

Compile Sass with JavaScript package#320

Merged
aduth merged 8 commits intomainfrom
aduth-build-sass
Apr 3, 2023
Merged

Compile Sass with JavaScript package#320
aduth merged 8 commits intomainfrom
aduth-build-sass

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Mar 31, 2023

This changes the Sass compilation to use the @18f/identity-build-sass package published through 18f/identity-idp.

Refer to GSA-TTS/identity-site#1000 and GSA-TTS/identity-site#1003 for rationale.

Additionally:

aduth added 4 commits March 31, 2023 13:52
I don't want to be installing dependencies each time I start the server
Not supported in this version, opted for inlining instead
@aduth aduth requested a review from zachmargolis March 31, 2023 18:22
@aduth aduth requested a review from a team as a code owner March 31, 2023 18:22
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 31 to 33
```sh
docker-compose run web bash
```
Copy link
Contributor

Choose a reason for hiding this comment

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

oh wow I forgot we had docker instructions here, are these still valid? should we remove? (maybe separate 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.

are these still valid? should we remove?

No idea, on either point 😅 The docker-compose.yml looks relatively straight-forward, so if it was working at some point, I assume (with low confidence) that it probably wouldn't be impacted by these changes.

}

//override for touchpoints survey included via script
#fba-modal-dialog .usa-banner__header-text {
Copy link
Contributor

Choose a reason for hiding this comment

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

we just removed touchpoints, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We did in #318, yes, with caveat that it "will likely return the survey after tax season". I'd be inclined to remove it as unused though. I can drop a comment on the old pull request about the removal to help with a potential future revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in b67bd9d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants