Skip to content

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
shadoath pushed a commit to rinsed-org/administrate that referenced this pull request Mar 12, 2025
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