Skip to content
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

Add components for Article #24

Merged
merged 5 commits into from
Sep 24, 2021
Merged

Conversation

rafayet-monon
Copy link
Contributor

This PR partially does some tickets component part-

What happened

Add components in articles

Insight

Added components for

  • Article list
  • Header
  • Button
  • posting details

Proof Of Work

Test cases should pass


module Base
module Button
class Component < ApplicationComponent
Copy link

@github-actions github-actions bot Sep 23, 2021

Choose a reason for hiding this comment

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

⚠️ Has 7 constants

@github-actions
Copy link

github-actions bot commented Sep 23, 2021

2 Warnings
⚠️ app/controllers/articles_controller.rb#L7 - Remove unused methods (articlescontroller#index)
⚠️ app/controllers/articles_controller.rb#L25 - Remove unused methods (articlescontroller#create)

Generated by 🚫 Danger

Comment on lines +26 to +28
@options = options

@options[:tag] ||= :a
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering a few days ago how to have a clean solution that accepts both <button> and <a> tags :)

That way is nice!
One question though: If the tag is 'button', should we remove the default value of the link param?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. Will do that,

Copy link
Member

@malparty malparty left a comment

Choose a reason for hiding this comment

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

One minor comment 👆, overall LGTM :)
Thanks a lot for that one, it gives a much better structure to components~ I'll start from this branch for login Font-End ;-)

@malparty
Copy link
Member

@rafayet-monon please pull the last commit on your local branch (d1c587d), I've added minor changes so that Unit Tests passes ;-)


private

def classes(type)
Copy link

@github-actions github-actions bot Sep 24, 2021

Choose a reason for hiding this comment

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

⚠️ Doesn't depend on instance state (maybe move it to another class?)

@junan junan merged commit 09f975d into development Sep 24, 2021
@junan junan deleted the feature/articles-view-component branch September 24, 2021 06:25
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.

3 participants