chore(cms): Update required Accounts CMS properties#19353
Conversation
| pageTitle: 'Test App - Custom Title', | ||
| }, | ||
| }; | ||
| } as RelierCmsInfo; |
There was a problem hiding this comment.
I'm not a huge fan of having to do this. But, because there are more required fields for the RelierCmsInfo interface, these don't meet those requirements. However, we don't need those properties for tests that use these so I just cast them to make typescript happy.
Perhaps there's a better way to do this?
nshirley
force-pushed
the
FXA-12286-accounts-required-properties
branch
from
93010e7 to
eaea417
Compare
| return this.cmsInfo && isCmsEnabled(this.cmsInfo) | ||
| ? this.cmsInfo | ||
| : undefined; | ||
| return this.cmsInfo; |
There was a problem hiding this comment.
Since all of these pages and properties are now required in Strapi we can't remove the checks here, just returning the cmsInfo. If the info exists, it's there to be used, if it doesn't everything still falls back to standard Accounts styling
There was a problem hiding this comment.
I updated the snapshot tests to use the same shared MOCK_CMS_INFO object these are just updates to fields that changed as a result. Looks like everything is still accurate though
nshirley
force-pushed
the
FXA-12286-accounts-required-properties
branch
2 times, most recently
from
a124a31 to
4c3104d
Compare
| alt="custom-header-logo" | ||
| class="h-auto w-[140px] mx-0" | ||
| src="https://example.com/snapshot-cms-logo.png" | ||
| src="https://gist.githubusercontent.com/dschom/857ebb2abd5f75937f211f1fd6bbf9a8/raw/33037c8905757a594e07eb29818d8519447fdec0/nightly-logo.svg" |
There was a problem hiding this comment.
why this file? I see dschom in the path.
There was a problem hiding this comment.
I updated the tests to use the same MOCK_CMS_INFO as some other tests. It's in src/pages/mocks and has a few links to those. Mostly I wanted to update the tests to be consistent since they were just using their own one off mocked cms info.
There was a problem hiding this comment.
This might have been from something I started, it turns out that github gist allows you to host svgs. Almost every other free hosting site does not allow this because of security concerns. We also did this before we had alot of images for cms in the cdn, now we do!
https://cdn.accounts.firefox.com/other/firefox-browser-logo.svg
There was a problem hiding this comment.
Thanks, @vbudhram ! I've got to resolve conflicts so I can update the mock to use that cdn image while I'm in there!
nshirley
force-pushed
the
FXA-12286-accounts-required-properties
branch
2 times, most recently
from
200302a to
74e8f4b
Compare
|
I'm going to hold on trying to rush this in to the end of the sprint. Given it changes requirements in Strapi of required fields and pages I want to double make sure we're good with this with Product (Ross) before merging. Recent pushes were just fixing merge conflicts and tests |
nshirley
force-pushed
the
FXA-12286-accounts-required-properties
branch
from
74e8f4b to
98d4e73
Compare
Because: - We want to make it clearer which pages and properties are required for Accounts CMS This Commit: - Makes several pages and properties of pages required for RelierCmsInfo - Updates references to these properties to remove null coalescing - Updates tests to use shared MOCK_CMS_INFO
nshirley
force-pushed
the
FXA-12286-accounts-required-properties
branch
from
98d4e73 to
96b9b4f
Compare
| }); | ||
|
|
||
| it('renders button with CMS passthrough', () => { | ||
| const cmsInfo: RelierCmsInfo = { |
3 participants
Because:
This Commit:
Issue that this pull request solves
Closes: #FXA-12286
Sister pr
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)