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

Tags Page with more details #435

Merged
merged 21 commits into from
Jan 18, 2024
Merged

Tags Page with more details #435

merged 21 commits into from
Jan 18, 2024

Conversation

panoramix360
Copy link
Collaborator

@panoramix360 panoramix360 commented Oct 24, 2023

This improves the Tags Page to have much more details on its table #396.

image

I moved some common methods between Stats <-> Tag to do the sorting on the Repo module, maybe we could create a new module for it, but let's see what you guys think.

Things missing

@nelsonic
Copy link
Member

Dedicated issue for sorting/filtering: #436

@nelsonic nelsonic added enhancement New feature or enhancement of existing functionality in-progress An issue or pull request that is being worked on by the assigned person technical A technical issue that requires understanding of the code, infrastructure or dependencies elixir Pull requests that update Elixir code labels Oct 24, 2023
@panoramix360 panoramix360 marked this pull request as ready for review November 14, 2023 00:18
@nelsonic nelsonic added the merge-conflicts The branch or pull request has merge conflicts with the target branch (e.g. master) label Nov 14, 2023
@panoramix360
Copy link
Collaborator Author

hey @nelsonic and @LuchoTurtle!

Finally, I was able to test the files!

Can you guys review it?

Can you help me with this?

  • I'm having trouble testing the tags_live, it seems that the creation of tags/items is not producing results on the tests, this is strange since the other live tests work fine. When I add tags to the DB it seems that it doesn't get added for the tests to work.
  • I fixed the merge conflicts but I saw that you guys created some modifications regarding lists on the tags controller/view and this might need some work to the index live view of the tags.
  • I notice that the test files are being ignored by the formatter on .formatter.exs, is this the intended behavior?

I'll wait for your answers!

Thank you and sorry to take a while to create tests for these.

@panoramix360
Copy link
Collaborator Author

@nelsonic

Forgot to mention that I saw that you commented some asserts on the stats_live tests and I fixed them! :D

@LuchoTurtle
Copy link
Member

Thanks for the amazing work @panoramix360 <3
I'm testing this and everything seems to be working perfectly!

image

Running mix c locally also works without a hitch. I'll take a quick look over the code and approve it :)

Copy link
Member

@LuchoTurtle LuchoTurtle left a comment

Choose a reason for hiding this comment

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

Other than my comment, I think this PR is just plain awesome 🥳.
I think the only thing missing is a simple write-up in the BUILDIT.md detailing the changes you've made in this PR. After that, we'll merge it instantly!

lib/app/repo.ex Outdated Show resolved Hide resolved
@panoramix360
Copy link
Collaborator Author

hey @nelsonic and @LuchoTurtle

I just created the new section on the docs with these modifications, but I'm unable to push the branch on this repo.

Should I fork it to add a pull request there? I think that I had the permissions earlier for this.

Please, let me know what I need to do, thanks!

@LuchoTurtle
Copy link
Member

@panoramix360 🤔 you should be able to at least create a PR. If you can't, maybe forking and creating a PR will be faster.
If that's too much work, you can simply add the section you've written to this repo's BUILDIT.md, which also works :)

@panoramix360
Copy link
Collaborator Author

great!

it's here!

dwyl/book#100

@LuchoTurtle
Copy link
Member

Thank you so much @panoramix360 !
Can you copy the text from https://github.com/dwyl/book/pull/100/files you've just created into the BUILDIT.md in this PR? Just for completeness :)
I'll approve it after you commit it.

Last stretch! 🏃 Thank you so, so much, again! 🙏

@panoramix360
Copy link
Collaborator Author

@LuchoTurtle, done!

@nelsonic nelsonic self-assigned this Jan 17, 2024
@nelsonic nelsonic added in-review Issue or pull request that is currently being reviewed by the assigned person and removed merge-conflicts The branch or pull request has merge conflicts with the target branch (e.g. master) labels Jan 17, 2024
Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

Excellent Additions @panoramix360 thanks again. 🙏

@nelsonic nelsonic assigned LuchoTurtle and unassigned nelsonic Jan 17, 2024
@nelsonic nelsonic added awaiting-review An issue or pull request that needs to be reviewed and removed in-review Issue or pull request that is currently being reviewed by the assigned person labels Jan 17, 2024
@nelsonic
Copy link
Member

"Merging can be performed automatically once the requested changes are addressed."

image

🤷‍♂️

@panoramix360
Copy link
Collaborator Author

Strange, the requested changes were already addressed

@nelsonic
Copy link
Member

Yeah, all seem to be resolved. Might need to manual override to merge. 💭

@nelsonic
Copy link
Member

I think it's because @LuchoTurtle requested the changes so it's "pending reviewer" ...

image

@nelsonic
Copy link
Member

I'm going to manual override as @LuchoTurtle is busy. 👌

image

@nelsonic nelsonic merged commit 55f46bc into main Jan 18, 2024
3 checks passed
@nelsonic nelsonic deleted the feature/tags-page-more-details branch January 18, 2024 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review An issue or pull request that needs to be reviewed elixir Pull requests that update Elixir code enhancement New feature or enhancement of existing functionality in-progress An issue or pull request that is being worked on by the assigned person technical A technical issue that requires understanding of the code, infrastructure or dependencies
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants