-
Notifications
You must be signed in to change notification settings - Fork 85
fix: location select mode and table display [ENG-1305] #6545
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
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
195893a
to
b8c9f28
Compare
fix: region rendering chore: revert table changes chore: update changelog chore: fix liniting
23b207c
to
079ce1e
Compare
const params = row.datasource_params as WebsiteMonitorParams | null; | ||
const locations = | ||
row.datasource_params && | ||
"locations" in row.datasource_params && | ||
Array.isArray(row.datasource_params.locations) | ||
? row.datasource_params.locations | ||
: []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we want to keep the cast to WebsiteMonitorParams
to avoid all the checking "locations" in row.datasource_params
, Array.isArray
etc ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Casting without checks could lead to a bug since we have no assurances that it's accurate.
The real fix is having the correct type for the table/response from the api that includes location
in the type.
const regionRecord = | ||
regionCode && PRIVACY_NOTICE_REGION_RECORD[regionCode[1]]; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's regionCode[1]
here? maybe that warrants a code comment :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it's def unclear.
These values are fallbacks that should be depreciated and will make note of it.
fides
|
Project |
fides
|
Branch Review |
main
|
Run status |
|
Run duration | 00m 58s |
Commit |
|
Committer | 3nder |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
0
|
|
0
|
|
0
|
|
5
|
View all changes introduced in this branch ↗︎ |
Closes ENG-1305
Description Of Changes
Fixing location select props and display within table
Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.md
updatedmain
downgrade()
migration is correct and works