Skip to content

Conversation

@dmitrytrager
Copy link
Collaborator

@dmitrytrager dmitrytrager commented Feb 3, 2025

What Issue Does This PR Cover, If Any?

Resolves #16

What Changed? And Why Did It Change?

Added model, seeds, migration, controller, views and tests to work with controllers

How Has This Been Tested?

With rspec request tests

Please Provide Screenshots

Screenshot 2025-02-03 at 20 48 30

Additional Comments

I've also introduced slim and rewrote some views to make them more verbose.
Though didn't add any language folder configuration for file storage

@dmitrytrager dmitrytrager marked this pull request as ready for review February 4, 2025 09:07
Gemfile Outdated
gem "stimulus-rails"
# Build JSON APIs with ease [https://github.com/rails/jbuilder]
gem "jbuilder"
gem "slim-rails"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should add this gem -- It adds additional complexity for new people/designers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This gem does not disable using erb for us

Copy link
Member

Choose a reason for hiding this comment

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

I know but having slim in the app makes it harder for newbies to contribute since they may not be familiar with slim/haml syntax.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe we could add some words into Readme saying that people can use what they best contribute with?

Copy link
Collaborator

@hernanvicente hernanvicente Feb 4, 2025

Choose a reason for hiding this comment

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

I agree with not adding more complexity. We should keep just one template DSL or format; it will avoid confusion or divergent styles for later markup implementations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My idea was that there is no complexity in CRUD application markup which Skillrx really is.
At the same time Erb template can be twice bigger and that makes things a bit slow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@seanmarcia @hernanvicente I've removed slim

db/structure.sql Outdated
@@ -0,0 +1,276 @@
SET statement_timeout = 0;
Copy link
Member

Choose a reason for hiding this comment

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

We also shouldn't move away from a schema.rb unless we absolutely have to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reverted this change as well

Copy link
Member

@seanmarcia seanmarcia left a comment

Choose a reason for hiding this comment

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

Can you rebase main in? Other than that it is good to go.

* main:
  Provider model implementation (#43)
@seanmarcia seanmarcia merged commit 6993258 into main Feb 4, 2025
4 checks passed
@dmitrytrager dmitrytrager deleted the language-model-16 branch February 4, 2025 13:58
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.

Language Model Implementation

4 participants