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

User Admin - refactored all breadcrumbs #694

Merged
merged 6 commits into from
Nov 9, 2018

Conversation

leofreitas
Copy link
Member

Hi,
I finished this module, and would started the module activies.

Copy link
Member

@SKuipers SKuipers left a comment

Choose a reason for hiding this comment

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

Wow, this is looking great!! 😃 I've highlighted a couple quick changes. Also, the __() translation function no longer required a $guid, so in your updated breadcrumbs can you remove any of the cases where the method begins with __($guid. I'll add this to the gist notes too. Thanks!

@SKuipers SKuipers changed the title modules/User Admin - refactored all breadcrumbs User Admin - refactored all breadcrumbs Nov 6, 2018
@rossdotparker
Copy link
Member

Wow, Leo you've been busy! I will leave look at this one tomorrow and aim to merge it in. Thanks : )

@leofreitas
Copy link
Member Author

leofreitas commented Nov 6, 2018

Hi Sandra and Ross,
I'm sorry, I don't know how I didn't see this! :-)

@SKuipers
Copy link
Member

SKuipers commented Nov 7, 2018

Thanks! Looks great. I've setup an issue so we can track the refactoring progress: #696

@yookoala
Copy link
Member

yookoala commented Nov 7, 2018

@SKuipers: Does the auto-generated breadcrumb object include the module name and path already? Or do we need to do a manual ->add(__(getModuleName($_GET["q"])), getModuleEntry($_GET["q"], $connection2, $guid)) in the middle (when refactoring the breadcrumb)?

@SKuipers
Copy link
Member

SKuipers commented Nov 7, 2018

Yep, it includes the module name & entry point already. Be sure to check out the instructions 😃

@rossdotparker
Copy link
Member

@leofreitas can you let me know when you feel this is ready to be merged, and I will take a look. Thanks!

@leofreitas
Copy link
Member Author

@rossdotparker this pullrequest is ready to be merge. thanks!

@rossdotparker
Copy link
Member

@leofreitas it looks good : ) but, you seem to have deleted uploads/.htaccess. Please can you restore this file, and then I will merge and test. Thanks!

@SKuipers
Copy link
Member

SKuipers commented Nov 8, 2018

🤦‍♀️ I missed that! Well spotted.

@leofreitas
Copy link
Member Author

Hi @rossdotparker,
I restored the folder uploads.
@SKuipers, I had removed breadcrumbs on delete action, but the travis show me a lot of fails and then I undid.

@rossdotparker
Copy link
Member

Hi Leo, thanks for persisting with this! The issue with the delete breadcrumbs is that the refactoring will cause a PHP error, which is worse than a Travis error. If you can remove them again, I'll then merge, and we can fix the tests to keep Travis happy. Sorry to create more work for you, hopefully it won't take long!

@SKuipers
Copy link
Member

SKuipers commented Nov 9, 2018

I've updated all the tests to no longer check for breadcrumbs on the delete action. Thanks!

@SKuipers SKuipers merged commit 5077772 into GibbonEdu:v17.0.00 Nov 9, 2018
@leofreitas leofreitas deleted the PR_669_breadcrumbs_user branch January 4, 2021 17:46
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.

4 participants