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.

Sign up for GitHub

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

Jump to bottom

Added Guitar Gallery #8

Merged
merged 10 commits into from
Nov 23, 2022
Merged

Added Guitar Gallery #8

merged 10 commits into from
Nov 23, 2022

Conversation

LiorKGOW
Copy link
Owner

@LiorKGOW LiorKGOW commented Oct 26, 2022 •
edited
Loading

This Component presents the guitars gotten from the server
It contains a sub-component: GuitarInfo which is a GalleryItem containing the info about a single guitar and presents it
In addition, this commit removes the unnecessary files regarding Users.js which is unused in this project

The first commit this PR contains is the hidden files which were not added somehow when I copied the files in the first place. It should be amended to the initialize Ruby On Rails commit.

Screenshot of the Guitar Gallery:
Screenshot from Guitar Store ver1

@LiorKGOW LiorKGOW self-assigned this Oct 26, 2022
@LiorKGOW LiorKGOW linked an issue Oct 26, 2022 that may be closed by this pull request
Guitars index page #6
Closed
2 tasks
This was referenced Oct 26, 2022
Open
Merged
Ron-Lavi
Ron-Lavi reviewed Oct 26, 2022
View reviewed changes
Copy link
Collaborator

@Ron-Lavi Ron-Lavi left a comment

Choose a reason for hiding this comment

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

Very nice work @LiorKGOW!!
Left some inline comments

.gitattributes Show resolved Hide resolved
.ruby-version Outdated Show resolved Hide resolved
app/controllers/guitar_gallery_controller.rb Outdated Show resolved Hide resolved
app/javascript/components/GuitarGallery.js Outdated Show resolved Hide resolved
app/controllers/guitar_gallery_controller.rb Outdated Show resolved Hide resolved
@LiorKGOW LiorKGOW added the Enhancement 🌟 New feature or option label Oct 27, 2022
@Ron-Lavi
Copy link
Collaborator

Added ESlint and Prettier setup in #13

@LiorKGOW LiorKGOW force-pushed the guitar-gallery branch 5 times, most recently from 6251e52 to 2aec5fb Compare November 2, 2022 07:34
@LiorKGOW
Copy link
Owner Author

LiorKGOW commented Nov 2, 2022

Rebased & repushed after #13 was merged 👍🏼

@LiorKGOW LiorKGOW added the Missing Tests This PR is missing appropriate testing label Nov 2, 2022
Ron-Lavi
Ron-Lavi reviewed Nov 2, 2022
View reviewed changes
Copy link
Collaborator

@Ron-Lavi Ron-Lavi left a comment

Choose a reason for hiding this comment

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

Almost there @LiorKGOW !

app/views/guitar_gallery/index.html.erb Outdated Show resolved Hide resolved
config/routes.rb Show resolved Hide resolved
app/javascript/components/GuitarGallery.js Show resolved Hide resolved
@LiorKGOW LiorKGOW force-pushed the guitar-gallery branch 5 times, most recently from f4b02fd to a4e8d0d Compare November 2, 2022 15:15
@LiorKGOW LiorKGOW mentioned this pull request Nov 3, 2022
Open
@LiorKGOW LiorKGOW added the Blocked By Another PR Another PR should be merged before this PR. The Blocking PR should be mentioned at the top comment label Nov 3, 2022
@LiorKGOW LiorKGOW force-pushed the guitar-gallery branch 3 times, most recently from 30c1914 to d79a86b Compare November 3, 2022 17:31
@LiorKGOW LiorKGOW mentioned this pull request Nov 9, 2022
Merged
3 tasks
@Ron-Lavi
Copy link
Collaborator

Hey @LiorKGOW,
if the tests PR in #18 is still not ready,
let's fix the conflicts here and merge this PR

@LiorKGOW
Copy link
Owner Author

Hey @LiorKGOW, if the tests PR in #18 is still not ready, let's fix the conflicts here and merge this PR

#18 was merged 👍🏼
Thanks @Ron-Lavi :)

@LiorKGOW
Undo this commit, it should still be worked on
baa23fc 
This commit consists several commits:
1. added the const URL file which is used in GuitarGallery
2. Added a Loader to be presented while the component is still being rendered
3. Added tests for GuitarGallery
Ron-Lavi
Ron-Lavi reviewed Nov 17, 2022
View reviewed changes
app/javascript/components/GuitarGallery.test.js Outdated Show resolved Hide resolved
jest.config.js Outdated Show resolved Hide resolved
@LiorKGOW LiorKGOW force-pushed the guitar-gallery branch 2 times, most recently from 2a48e74 to 84c4560 Compare November 22, 2022 17:25
@LiorKGOW
Exporting fixutes of const data used in the tests of GuitarGallery
7c1d677 
Exporting fixutes of const guitars data used in the tests of the GuitarGallery to an external fixtures file
@LiorKGOW LiorKGOW added Question ❓ Further information is requested and removed Bug 🐛 Something isn't working Missing Tests This PR is missing appropriate testing Blocked By Another PR Another PR should be merged before this PR. The Blocking PR should be mentioned at the top comment labels Nov 23, 2022
@LiorKGOW
Copy link
Owner Author

I have managed to resolve the rubocop issue: the function "guitarController" was too long the max lines of a function is 10 lines with adding the following comments to the file:
at the beginning:
# rubocop:disable Metrics/MethodLength
and at the end:
# rubocop:enable Metrics/MethodLength

Do you think this is good practice? @MariaAga @Ron-Lavi
I have found it here: ifmeorg/ifme#1866
Solving this issue: ifmeorg/ifme#1863
But I wasn't sure how to add that to the github/workflows/rubocop.yml file, cause our file is formatted a bit different
So I used the solution he replaced, but it worked anyway haha

@Ron-Lavi
Copy link
Collaborator

@LiorKGOW I see, it's because of the mock server data.. imho it's totally fine in this case 👍🏼

@LiorKGOW LiorKGOW removed the Question ❓ Further information is requested label Nov 23, 2022
@LiorKGOW LiorKGOW added the Ready to Merge 🤩 This PR is ready to be merged into the main branch label Nov 23, 2022
Ron-Lavi
Ron-Lavi approved these changes Nov 23, 2022
View reviewed changes
Copy link
Collaborator

@Ron-Lavi Ron-Lavi left a comment

Choose a reason for hiding this comment

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

Thanks @LiorKGOW !! Great work :)

@Ron-Lavi Ron-Lavi merged commit f909685 into main Nov 23, 2022
@LiorKGOW LiorKGOW deleted the guitar-gallery branch December 20, 2022 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ron-Lavi Ron-Lavi Ron-Lavi approved these changes

Assignees

@LiorKGOW LiorKGOW

Labels
Enhancement 🌟 New feature or option Ready to Merge 🤩 This PR is ready to be merged into the main branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Guitars index page
2 participants
@LiorKGOW @Ron-Lavi