-
Notifications
You must be signed in to change notification settings - Fork 130
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
feat: Leetcode Company Tagged 🗂️ #530
base: main
Are you sure you want to change the base?
Conversation
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.
Great work @cjohnson74! Just left some comments regarding changing some naming, but otherwise the code and the functionality looks great to me!
@@ -154,6 +155,10 @@ export default function CompanyPage() { | |||
{company.levelsFyiSlug && ( | |||
<LevelsFyiLink slug={company.levelsFyiSlug} /> | |||
)} | |||
|
|||
{company.leetcodeTagged && ( | |||
<LeetcodeTaggedLink leetcode_tagged={company.leetcodeTagged} /> |
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.
let's change the naming from leetcode_tagged
to slug
, so we can have consistency with the pattern used for the Levels.fyi link as well
export async function up(db: Kysely<any>) { | ||
await db.schema | ||
.alterTable('companies') | ||
.addColumn('leetcode_tagged', 'varchar') |
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.
let's change leetcode_tagged
to leetcode_tagged_slug
to follow the pattern that's used with levels_fyi_slug
fetching updates
…eetcode-tagged pulling changes
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.
We just need to complete the refactoring of the name (from leetcodeTagged
to leetcodeTaggedSlug
) in a couple more places and then this will be good to go!
@@ -46,6 +46,7 @@ export async function loader({ params, request }: LoaderFunctionArgs) { | |||
'companies.imageUrl', | |||
'companies.name', | |||
'companies.levelsFyiSlug', | |||
'companies.leetcodeTagged', |
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.
Since we're changing the name to leetcodeTaggedSlug in the database, we need to have that changed reflected here
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.
Done
{company.leetcodeTagged && ( | ||
<LeetcodeTaggedLink slug={company.leetcodeTagged} /> |
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.
we also need to change the name in these two lines
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.
And done!
…eetcode-tagged Pulling to push
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.
LGTM! 🚀
Description ✏️
Closes #516
Type of Change 🐞
Checklist ✅