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 dynamic html id to Matestack pages #551

Merged
merged 3 commits into from
Jun 8, 2021
Merged

Add dynamic html id to Matestack pages #551

merged 3 commits into from
Jun 8, 2021

Conversation

marcosvafilho
Copy link
Contributor

@marcosvafilho marcosvafilho commented Jun 3, 2021

Issue #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

  • Add a page_id method to build a html id string from controller and action values provided by params.
  • 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
Copy link
Member

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!

Copy link
Member

@jonasjabari jonasjabari left a comment

Choose a reason for hiding this comment

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

Looks good to me! As said before, we just need a little spec covering this feature :)

@pascalwengerter
Copy link
Contributor

Love to see this implemented!

@marcosvafilho
Copy link
Contributor Author

Hey guys, I'll happily add tests :)

@pascalwengerter
Copy link
Contributor

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
Copy link
Contributor Author

Added both tests and documentation reference.

@marcosvafilho marcosvafilho requested a review from jonasjabari June 6, 2021 01:10
@pascalwengerter
Copy link
Contributor

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

Copy link
Member

@jonasjabari jonasjabari left a comment

Choose a reason for hiding this comment

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

Great stuff!

@jonasjabari jonasjabari changed the base branch from main to next_release June 8, 2021 08:35
@jonasjabari jonasjabari merged commit 84eed8b into matestack:next_release Jun 8, 2021
@jonasjabari
Copy link
Member

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
Copy link
Contributor Author

No problem :)

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

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.

Add dynamically generated page-content-name id/class
3 participants