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

Change layout on profile pages #14420

Open
wants to merge 8 commits into
base: 3.x
from

Conversation

Projects
None yet
7 participants
@GulomovCreative
Copy link
Contributor

commented Feb 22, 2019

What does it do?

  1. Changes layout on user edit page
  2. Changes layout on profile edit page (General Information Tab)
  3. Changes layout on profile edit page (Reset Password Tab)
  4. Outputs missing fields on profile edit page: Address, City, Website and Gender (but I could not output
    country field, some problem with modx-combo-country. If you know the solution, send it)
  5. Adds username to header on profile edit page

UPD:

  1. Logical resort of fields
  2. Adds username to title
  3. Adds changing header during keyup username field

Before

before

After

after

Before

before2

After

after2

Before

before3

After

after3

Before

before4

After

after4

Why is it needed?

At least saving space

Related issue(s)/PR(s)

#13988

@GulomovCreative GulomovCreative requested review from Mark-H and opengeek as code owners Feb 22, 2019

@Jako

This comment has been minimized.

Copy link
Collaborator

commented Feb 22, 2019

What is your issue with the country combo?

@Ruslan-Aleev

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

@Jako The name of the country is not displayed, i.e. you can change the country and it will be saved, but the name itself will not be visible after the page is reloaded.

country

@Jako

This comment has been minimized.

Copy link
Collaborator

commented Feb 22, 2019

Maybe this will be solved, when 2.x is merged into 3.x. If not, it could be fixed quite easy.

@GulomovCreative

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

@Jako There is still a problem with the date output. Pay attention to the date format, here I need help

No more problem with datefield, I forgot about the format property

@JoshuaLuckers

This comment has been minimized.

Copy link
Collaborator

commented Feb 22, 2019

We have to wait till 2.x is merged into 3.x (which is in progress).

@GulomovCreative

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

We have to wait till 2.x is merged into 3.x (which is in progress).

Yeah, @Alroniks told us

@GulomovCreative

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

@Ruslan-Aleev thx for resort and ideas

@Mark-H
Copy link
Collaborator

left a comment

Needs some XSS protection.

@GulomovCreative

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

Needs some XSS protection.

Good remark. I will fix fixed, but it turns out that there are the same problems in FC and user.update class

public function getPageTitle() {
if($this->user == null) {
return $this->modx->lexicon('user_err_nf');
} else {
return $this->modx->lexicon('user').': '.$this->user->get('username');
}
}

,listeners: {
'keyup': {scope:this,fn:function(f,e) {
Ext.getCmp('modx-fcp-header').getEl().update(_('profile')+': '+f.getValue());
}}
}

@@ -40437,7 +40437,7 @@ Ext.form.Field = Ext.extend(Ext.BoxComponent, {
if(v === this.emptyText || v === undefined){
v = '';
}
return v;
return Ext.util.Format.htmlEncode(v);

This comment has been minimized.

Copy link
@Mark-H

Mark-H Mar 5, 2019

Collaborator

While this is clever, I don't think we should apply htmlEncode to the getValue method in this way (assuming that's where you added that, GitHub wont show me more context because it's a too large file).

There's nothing wrong with a value including some good ol' HTML (we don't want all content to suddenly be html encoded for example, people do use RTEs), as long as it's is escaped based on the specific places that uses it. Inserting a value into a header is such a place that needs special treatment.

This comment has been minimized.

Copy link
@GulomovCreative

GulomovCreative Mar 9, 2019

Author Contributor

Oh, i missed that part

This comment has been minimized.

Copy link
@Mark-H

Mark-H May 3, 2019

Collaborator

Looks like this still needs to be reverted.

This comment has been minimized.

Copy link
@GulomovCreative

GulomovCreative Jun 15, 2019

Author Contributor

@Mark-H I reverted and fixed

@sdrenth

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

@GulomovCreative Great work on improving the profile pages! I'm sorry I didn't see your PR so I made this PR which is related to yours.

I think it would be great if my PR gets somewhat merged into yours if possible. I think the update profile view is still a bit unclear and I would like to know your opinion about my approach for this by separating all fields into two columns.

Also I think that the changes for moving change password and the position of the save button would be a nice addition to this pull request.

I'd like to know what you think of this and how I can help with this.

EDIT:
Totally forgot to mention my PR:
#14578

I've added a screenshot in my PR which shows the two column layout and that the change password tab has been removed (Has been moved to the bottom right corner of the "General Information" tab.

@sdrenth

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

@GulomovCreative Any thoughts on my last comment?

@gpsietzema

This comment has been minimized.

Copy link
Collaborator

commented Jun 7, 2019

@GulomovCreative Reminder.

@GulomovCreative

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

@sdrenth Hi, i was busy. But now i am here

@GulomovCreative

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

@sdrenth I like your implementation. But I don't want to mess around with conflicts, if i try to merge your commits. You do not mind, then I just make my commits with your code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.