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

Use position for element dom_id #2368

Merged
merged 3 commits into from Sep 4, 2022

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented Sep 2, 2022

What is this pull request for?

Change the DOM ID of elements to use position instead of id

We cannot use the id, because that will change if publish the page. The position will be the same across versions and combined with the name it is unique on each page.

Deprecate element_dom_id helper

Use element.dom_id instead.

Deprecate full_url_for_element helper

Please element.dom_id and pass it as anchor to your URLs.

Refs #2354

Checklist

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have added tests for the changes

@tvdeyen tvdeyen changed the title Deprecate element_dom_id helper Deprecate implicit element dom ids Sep 2, 2022
@tvdeyen tvdeyen mentioned this pull request Sep 2, 2022
2 tasks
@tvdeyen tvdeyen force-pushed the deprecate-element-dom-id branch 2 times, most recently from 923bef1 to bfcd895 Compare September 2, 2022 20:15
@tvdeyen tvdeyen force-pushed the deprecate-element-dom-id branch 2 times, most recently from bdd0fe5 to ea72966 Compare September 3, 2022 20:20
@tvdeyen tvdeyen changed the title Deprecate implicit element dom ids Use position for element dom_id Sep 3, 2022
@tvdeyen tvdeyen requested a review from a team September 3, 2022 20:25
Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

What's with the position of nested elements? Could there be conflicts?

@tvdeyen
Copy link
Member Author

tvdeyen commented Sep 4, 2022

What's with the position of nested elements? Could there be conflicts?

Yes, if the same element is also nested into another element. We could add the parents dom_id.

Also, I thought ultimately we could add a DomId class that people could configure to their needs.

@tvdeyen
Copy link
Member Author

tvdeyen commented Sep 4, 2022

@mamhoff the parent elements dom_id (if present) is now included. #2373 introduces the possibility to use a custom dom id class.

We cannot use the id, because that will change if publish the page.
The position will be the same across versions and combined with
the name it is unique on each page.

Fixes AlchemyCMS#2354
Use `element.dom_id` instead.
Please implement your own unique DOM ID for your elements and
pass it as anchor to your URLs.
@tvdeyen tvdeyen merged commit 56e95a0 into AlchemyCMS:main Sep 4, 2022
@tvdeyen tvdeyen deleted the deprecate-element-dom-id branch September 4, 2022 16:15
@dbwinger
Copy link
Contributor

@tvdeyen this breaks links using anchors, right?

@tvdeyen
Copy link
Member Author

tvdeyen commented Aug 11, 2023

@dbwinger no, it makes them work again. It stopped working after we introduced page versions and didn't realized that the database ID used as DOM ID does not work anymore.

Existing links have been broken since then and this fixes the issue for new links created.

This behavior is now configurable and can be adjusted to your liking as well.

Hopefully this works for you

@dbwinger
Copy link
Contributor

Got it. Yes, this will work. I think I can easily migrate my broken links.

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.

None yet

3 participants