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.

Sign up for GitHub

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

Jump to bottom

[Help editor] Remove parentID #7025

Merged
merged 4 commits into from
Oct 6, 2020
Merged

[Help editor] Remove parentID #7025

merged 4 commits into from
Oct 6, 2020

Conversation

laemtl
Copy link
Contributor

@laemtl laemtl commented Sep 10, 2020 •
edited
Loading

According to #5784, parents and subtopics are not relevant anymore in the help editor. This PR removes the concept of hierarchy for the help content.

To test

@laemtl laemtl force-pushed the 2020-09-09-help-editor-remove-parentID branch from 75b97fe to ef81293 Compare September 10, 2020 02:48
@laemtl laemtl force-pushed the 2020-09-09-help-editor-remove-parentID branch from ef81293 to 452cbb3 Compare September 10, 2020 04:36
@maltheism maltheism self-requested a review September 14, 2020 15:13
maltheism
maltheism approved these changes Sep 14, 2020
View reviewed changes
Copy link
Member

@maltheism maltheism left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The code changes look good.

@driusan
Copy link
Collaborator

driusan commented Sep 14, 2020

@maltheism did you test or just review?

@maltheism
Copy link
Member

maltheism commented Sep 14, 2020 •
edited
Loading

@driusan I just reviewed the code and visited the module.

@maltheism
Copy link
Member

@driusan @laemtl maybe the CHANGELOG.md should be updated?

@ridz1208
Copy link
Collaborator

@pierre-p-s can you make sure to test the actual help displayed in the modules (not only the help editor module itself)

@laemtl
Copy link
Contributor Author

laemtl commented Sep 21, 2020

@maltheism Good point.

@pierre-p-s
Copy link
Contributor

Tested, I dont see any more mention of a parent ID.
I am still getting some PHP notices though (screenshot below)

Screen Shot 2020-09-21 at 2 37 20 PM

@laemtl laemtl mentioned this pull request Sep 23, 2020
Merged
@laemtl laemtl force-pushed the 2020-09-09-help-editor-remove-parentID branch 2 times, most recently from 16bc626 to 1a1c385 Compare September 24, 2020 17:16
@laemtl
Copy link
Contributor Author

laemtl commented Sep 24, 2020 •
edited
Loading

Thanks @pierre-p-s, I fixed the notices.

@laemtl laemtl force-pushed the 2020-09-09-help-editor-remove-parentID branch 3 times, most recently from 01c38dc to d0ce675 Compare September 24, 2020 17:40
@laemtl laemtl force-pushed the 2020-09-09-help-editor-remove-parentID branch 2 times, most recently from 1180b29 to dc49727 Compare September 24, 2020 17:49
@laemtl laemtl force-pushed the 2020-09-09-help-editor-remove-parentID branch from dc49727 to a4ad82a Compare September 24, 2020 19:20
driusan
driusan reviewed Oct 5, 2020
View reviewed changes
CHANGELOG.md Outdated
@@ -25,6 +25,8 @@ requesting a new account and will be displayed in the User Accounts module (PR #
#### Bug Fixes
- *Add item here*
### Modules
#### Help Editor
- parentID was removed. (PR #7025)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this changelog update means anything to users.

Copy link
Contributor Author

@laemtl laemtl Oct 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@driusan What do you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

how about a few more words to make it meaningful to readers, like :

Cleaned up deprecated column parentID #7025

Copy link
Collaborator

Choose a reason for hiding this comment

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

I still don't think that means anything to readers. What's the effect of removing parentID? If it's not noticeable in any way, it's not worth mentioning in the changelog.

Copy link
Contributor Author

@laemtl laemtl Oct 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It affects the rendering of the data table, this change removes the column Parent Topic
Screenshot from 2020-10-05 15-22-21

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@driusan I edited the CHANGELOG to add @christinerogers suggestion, let me know if you prefer to remove the mention.

@laemtl laemtl force-pushed the 2020-09-09-help-editor-remove-parentID branch from 4534e0b to 2f0ec21 Compare October 5, 2020 19:35
@laemtl laemtl force-pushed the 2020-09-09-help-editor-remove-parentID branch from 2f0ec21 to a0fca07 Compare October 5, 2020 19:36
@christinerogers christinerogers added the Cleanup PR or issue introducing/requiring at least one clean-up operation label Oct 5, 2020
christinerogers
christinerogers approved these changes Oct 5, 2020
View reviewed changes
Copy link
Contributor

@christinerogers christinerogers left a comment

Choose a reason for hiding this comment

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

Looks fine and seems well tested. Thanks for the extra context in the Changelog entry.

@driusan i think it's ready for your re-review.

@driusan driusan merged commit 11e6780 into aces:main Oct 6, 2020
@laemtl laemtl deleted the 2020-09-09-help-editor-remove-parentID branch October 13, 2020 20:15
spell00 pushed a commit to spell00/Loris that referenced this pull request Nov 4, 2020
@laemtl @spell00
[Help editor] Remove parent topic (aces#7025)
d8a1b93 
Remove unused Parent Topic hierarchy from help system.

Resolves aces#5784
@ridz1208 ridz1208 added this to the 24.0.0 milestone Nov 27, 2020
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 29, 2021
@laemtl @AlexandraLivadas
[Help editor] Remove parent topic (aces#7025)
7317216 
Remove unused Parent Topic hierarchy from help system.

Resolves aces#5784
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driusan driusan driusan left review comments

@christinerogers christinerogers christinerogers approved these changes

@maltheism maltheism maltheism approved these changes

@pierre-p-s pierre-p-s pierre-p-s approved these changes

Assignees

@pierre-p-s pierre-p-s

Labels
Cleanup PR or issue introducing/requiring at least one clean-up operation
Projects
None yet
Milestone
24.0.0
Development

Successfully merging this pull request may close these issues.

[Help Editor] parent and subtopics?
6 participants
@laemtl @driusan @maltheism @ridz1208 @pierre-p-s @christinerogers