Add dynamic html id to Matestack pages

marcosvafilho commented 3 years ago

Issue https://github.com/matestack/matestack-ui-core/issues/435: Add dynamically generated page-content-name id/class

As pointed by @pascalwengerter on issue #435, it would be nice to have a dynamic id attribute on the Matestack::Ui::Core::Page root element, allowing us to style pages without adding a wrapping element, and directly target the id attribute with CSS rules instead.

This PR is an attempt to bring such improvement. Let me know if it's good enough, otherwise I can work to make it better with your feedback/input.

Changes

[x] Add a page_id method to build a html id string from controller and action values provided by params.
[x] Use the page_id method to populate the id argument within div helpers used inside Matestack::Ui::Core::Page.

Notes

Using dasherize to comply with the CSS naming pattern already in place.
PR template says to point to the develop branch, but it looks like it doesn't exist anymore. Excuse me if I'm making a mistake by opening a PR to the main branch instead.
I'm still at the very beginning exploration phase of Matestack, but I would like to thank you all for bringing such a nice gem to life.

closes #435

jonasjabari commented 3 years ago

Hey @marcosvafilho! Thank you so much for your PR! We're currently about to define the branching workflow, it's not properly set up right now. So no worries about the branch ;) I will create a "next_release" branch from the main branch and will target your PR to that branch then. Speaking about your PR: The implementation looks good to me! We just would need a tiny little spec, checking if the IDs are properly applied to pages, in order to not loose this feature sometime in the future. Are you familiar with Rspec/Capybara? We've created a small doc on how to set up the dockerized test env locally: https://docs.matestack.io/matestack-ui-core/community/contribute Would you be able to add a spec for your feature following a spec like this one: https://github.com/matestack/matestack-ui-core/blob/main/spec/test/base/core/page/controller_instance_access_spec.rb? (Just duplicate that file and adjust the routes in order to have unique routes for your spec) Besides that: Are you on Discord? Would love to get to know you better and help you along your journey into using/contributing to Matestack :) Cheers!

pascalwengerter commented 3 years ago

Love to see this implemented!

marcosvafilho commented 3 years ago

Hey guys, I'll happily add tests :)

pascalwengerter commented 3 years ago

And maybe a small hint&example about this behavior somewhere in the matestack-pages docs? As you already said the generated IDs could help with styling and testing so ppl should be able to find it in the documentation 🤓

marcosvafilho commented 3 years ago

Added both tests and documentation reference.

pascalwengerter commented 3 years ago

Changes look very good to me, needs approval by @jonasjabari to get it merged though ;)

jonasjabari commented 3 years ago

Hey @marcosvafilho I need to revert this PR as discussed in #554. I want to do a release today (2.1) and therefore need to get rid of the issue which was introduced in this PR (sorry for overseeing it while reviewing). Once the issue is resolved, I happily merge this PR again and release this feature within the next release (2.2) :)

If you need any help fixing the bug (see #554), let me know!

Cheers, Jonas

marcosvafilho commented 3 years ago

No problem :)

I'll work on the other PR later this week :)

matestack / matestack-ui-core