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

SS4 improvements #138

Merged
merged 13 commits into from
Apr 30, 2018
Merged

Conversation

JorisDebonnet
Copy link
Contributor

@JorisDebonnet JorisDebonnet commented Feb 21, 2018

Work in progress.

  1. New folder src/Pages
    I moved the pages into a new src/Page folder, to make the structure make more sense and so that MemberProfilePage and MemberProfilePageController sit together.

  2. Refactored templates
    I made the default templates work again by refactoring them into the default way templates are handled in SS4, in those namespaced folders. This is of course a backwards incompatible change, but I guess we have those anyway. It's easy to move from the old to the new template structure.
    (note: I have MemberProfilePage_addmembers instead of _add because for some inexplicable reason I couldn't get it to work with just _add)

  3. Fixed the CMS page icon

  4. Fixed the Password fields (front-end)
    SS4 no longer has the separate ConfirmedPasswordField.js file; it's all in one bundle now. So I just took that little piece of .js and .css out and placed it in this repository. About the disappearing "New Password" label, I couldn't exactly figure out the source of the problem, but worked around it.

  5. Fixed MemberProfileValidator

@JorisDebonnet JorisDebonnet changed the title SS-4 improvements SS4 improvements Feb 21, 2018
@JorisDebonnet JorisDebonnet force-pushed the ss4-improvements branch 2 times, most recently from 1fce59e to 2cf1515 Compare February 23, 2018 05:55
@JorisDebonnet
Copy link
Contributor Author

JorisDebonnet commented Feb 23, 2018

Allright, I think this is about as far as I personally need it. The email seems to correctly parse $Member.Email now, although I didn't actually research that issue; perhaps because of the Validator fix.

I would suggest also formatting the code to PSR-2 and turning array() into []. I can still do this at a later time, if you like.

@JorisDebonnet JorisDebonnet force-pushed the ss4-improvements branch 6 times, most recently from dae25eb to 292eb48 Compare February 23, 2018 07:17
@JorisDebonnet
Copy link
Contributor Author

(I tried real hard to make those template refactor commits cleaner because it's mostly just moved stuff, not so much deleted and added, but it just won't take...)

@@ -0,0 +1,14 @@
/* extracted from silverstripe/framework */
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this not something that we can reference from silverstripe/framework, rather than copy and pasting it across here?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the main thing that jumps out to me from the PR. In regards to your PSR-2 comments, I'm planning to run PSR-2 formatting across all of our modules in the near future (hooking them up to Travis to enforce this).

Copy link
Contributor Author

@JorisDebonnet JorisDebonnet Feb 26, 2018

Choose a reason for hiding this comment

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

I understand the issue, but this code does not appear in a separate .js file anymore. SS4 now bundles nearly all cms js in a single .js file. I tried searching for showOnClickContainer in the vendor/silverstripe folder, but it's only to be found in admin/client/dist/js/bundle.js. I remember reading about this in the upgrade guide:
https://docs.silverstripe.org/en/4/changelogs/4.0.0#js-in-framework

It suggests transpiling source es6 files in our own build process, but I can't find those source files. Perhaps they're not shipped to the packagist release or something? So in that case, since it's only a small piece of .js, I thought I'd just integrate it directly...

("extracted from silverstripe/framework" actually means I got it from the SS3 code; nowadays, it is actually found in silverstripe/admin, so perhaps I should change that comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah okay, that makes sense. It feels like the confirm password field was never intended for the front end. In which case, happy for this to stay until we come up with a better solution. Thanks for all your work on this PR. I'll probably have a quick play with it before merging :)

@JorisDebonnet
Copy link
Contributor Author

I can't get the Members List to work... it stays empty. The problem is in MemberProfileViewer's handleList, where ->relation('Members') does not return any Members. I noticed that the API documentation says "For it to work, the relation must be defined on the data class that you used to create this DataList." so just in case it didn't lke starting out from MemberProfilePage_Groups, I tried writing the query manually as below, but I still get an empty result when I call ->relation('Members') on that.

Group::get()
    ->innerJoin('MemberProfilePage_Groups', 'MemberProfilePage_Groups.GroupID = Group.ID')
    ->where("MemberProfilePage_Groups.MemberProfilePageID = '{$this->parent->ID}'");
// ->relation('Members') on this is still empty

According to /admin/security/ I do have Members in the Group I'm using though ... (I'm just using the ContentEditors Group to test)

Do you have any idea?

(in the meantime I'm just continuing my app development while not using any Group -- I added a commit for that here, in order to allow listing all members if no Group is selected)

@JorisDebonnet JorisDebonnet force-pushed the ss4-improvements branch 2 times, most recently from f36d906 to ae27af9 Compare February 27, 2018 05:09
@JorisDebonnet JorisDebonnet force-pushed the ss4-improvements branch 2 times, most recently from 8fd0564 to aa14962 Compare March 21, 2018 02:43
@JorisDebonnet JorisDebonnet force-pushed the ss4-improvements branch 2 times, most recently from 85cc97f to d280f65 Compare April 29, 2018 23:01
@JorisDebonnet
Copy link
Contributor Author

  1. Install into vendor folder
  2. Slightly updated README.md
  3. Various other fixes

@silbinarywolf
Copy link
Contributor

@JorisDebonnet Looks great, thanks for your work!
Will merge this in and cleanup / test / fix whatever is remaining :)

@silbinarywolf silbinarywolf merged commit 5093709 into symbiote:master Apr 30, 2018
@JorisDebonnet
Copy link
Contributor Author

Cool :)
You might have noticed I raised an issue at silverstripe-framework about DataList::relation which is preventing the MemberView from working properly. But if necessary we can fix that in a future commit.

@JorisDebonnet JorisDebonnet deleted the ss4-improvements branch May 1, 2018 00:48
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.

3 participants