-
Notifications
You must be signed in to change notification settings - Fork 41
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
[V2] Technology #49
[V2] Technology #49
Conversation
adcca7a
to
0cd8de6
Compare
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.
Looks great! Just some minor feedback
app/models/company_technology.rb
Outdated
belongs_to :technology | ||
|
||
validates :company_id, uniqueness: { scope: :technology_id } | ||
validates :technology_id, uniqueness: { scope: :company_id } |
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.
These two are redundant. Removing one of them should achieve the same result.
app/models/technology.rb
Outdated
|
||
validates :name, presence: true, uniqueness: true | ||
validates :background_color, | ||
format: { with: /\A#(?:[0-9a-fA-F]{3}){1,2}\z/, message: "must be a valid hex color code" }, |
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.
Can we extract the regex into a constant?
class Technology < ApplicationRecord
HEX_COLOR_REGEX = /\A#(?:[0-9a-fA-F]{3}){1,2}\z/.freeze
# ...
validates :background_color,
format: { with: HEX_COLOR_REGEX, ... }
We can move it to a custom validator once/if we need it in another model.
db/schema.rb
Outdated
@@ -19,8 +19,28 @@ | |||
t.string "website" | |||
t.datetime "created_at", null: false | |||
t.datetime "updated_at", null: false | |||
t.text "introduction" |
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.
Can this be removed? I think it's a leftover from your local development database. Removing it manually from db/schema.rb
is fine.
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.
👍
08ef0e0
to
42e185d
Compare
This pull request introduces a new feature to associate companies with technologies, allowing users to view technologies associated with each company. The most important changes include updates to the models, serializers, and frontend components to support this new feature.