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

Enable to pass html options to URL field #2139

Merged
merged 1 commit into from
Feb 15, 2022

Conversation

shouichi
Copy link
Contributor

@shouichi shouichi commented Feb 8, 2022

Close #2134.

@shouichi shouichi force-pushed the add-html-options-to-url branch from 7e21875 to de79b76 Compare February 8, 2022 03:28
Copy link
Member

@nickcharlton nickcharlton left a comment

Choose a reason for hiding this comment

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

This looks good!

One thought though: Do you think we could move the options up to the Base class so that it applies to all fields?

@shouichi
Copy link
Contributor Author

shouichi commented Feb 9, 2022

One thought though: Do you think we could move the options up to the Base class so that it applies to all fields?

Sounds cool! I searched the codebase (git grep content_tag) and found that only Fields::Url uses content_tag. The templates of some fields are somewhat non-trivial. It might not be obvious where the html_options applies to. I believe we can gradually add html_options to certain fields, then introduce it to the base class when we find it useful (and we can do that in a backward-compatible way I guess). What do you think?

By the way, I realized that I also needed to change _index.html.erb template too. I'll fix it tomorrow.

Thanks!

@shouichi shouichi force-pushed the add-html-options-to-url branch from de79b76 to ebf5a86 Compare February 10, 2022 01:58
Copy link
Member

@nickcharlton nickcharlton left a comment

Choose a reason for hiding this comment

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

Thanks for updating!

Yes, I think we can leave it as just the URL field for now and expand it over time.

@nickcharlton nickcharlton merged commit 13d4099 into thoughtbot:main Feb 15, 2022
@shouichi shouichi deleted the add-html-options-to-url branch February 18, 2022 01:33
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.

Support opening url in a new browser tab
2 participants