Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Add UI in settings to change ID Server #3300

Merged
merged 27 commits into from
Aug 14, 2019
Merged

Add UI in settings to change ID Server #3300

merged 27 commits into from
Aug 14, 2019

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Aug 9, 2019

Screenshot 2019-08-12 at 14 42 08

Just changes the current ID server being used

To come in subsequent PRs:

  • Store this in account data
  • Check for terms or support the proper UI for accepting terms when setting
  • Support disconnecting

Part 1 of element-hq/element-web#10094
Requires matrix-org/matrix-js-sdk#1013

dbkr added 2 commits August 9, 2019 18:07
Just changes the current ID server being used

To come in subsequent PRs:
 * Store this in account data
 * Check for terms or support the proper UI for accepting terms when setting
 * Support disconnecting

Part 1 of element-hq/element-web#10094
Requires matrix-org/matrix-js-sdk#1013
@dbkr
Copy link
Member Author

dbkr commented Aug 12, 2019

Tests do actually pass when rich isn't breaking the build agents

@dbkr dbkr requested a review from a team August 12, 2019 12:00
@dbkr dbkr removed the request for review from a team August 12, 2019 13:17
@dbkr
Copy link
Member Author

dbkr commented Aug 12, 2019

oops, don't merge this yet. Keen observers can play Spot The Deliberate Mistake for a bit.

Pretty important that we don't send that to the new IS...
@dbkr dbkr mentioned this pull request Aug 12, 2019
@dbkr dbkr requested a review from a team August 12, 2019 13:56
@dbkr
Copy link
Member Author

dbkr commented Aug 12, 2019

ok, better

@jryans jryans requested review from jryans and removed request for a team August 12, 2019 15:00
Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Thanks, this is coming together nicely!

Will there be a future part focused on suggesting 3PIDs should be unbound first: element-hq/element-web#10094 (comment)?

res/css/views/settings/_SetIdServer.scss Outdated Show resolved Hide resolved
src/components/views/settings/SetIdServer.js Outdated Show resolved Hide resolved
src/components/views/settings/SetIdServer.js Outdated Show resolved Hide resolved
src/components/views/settings/SetIdServer.js Outdated Show resolved Hide resolved
src/components/views/settings/SetIdServer.js Outdated Show resolved Hide resolved
src/components/views/settings/SetIdServer.js Outdated Show resolved Hide resolved
src/components/views/settings/SetIdServer.js Outdated Show resolved Hide resolved
src/components/views/settings/SetIdServer.js Outdated Show resolved Hide resolved
src/components/views/elements/Field.js Outdated Show resolved Hide resolved
@@ -135,6 +138,7 @@ export default class Field extends React.PureComponent {

render() {
const { element, prefix, onValidate, children, ...inputProps } = this.props;
delete inputProps.tooltip; // needs to be removed from props but we don't need it here
Copy link
Collaborator

Choose a reason for hiding this comment

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

delete forces the JIT to deoptimise the object, but probably doesn't matter for this case...

Copy link
Member Author

Choose a reason for hiding this comment

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

interesting - I started with a nop !tooltip although it sort of seems awful.

*/

.mx_SetIdServer .mx_Field_input {
width: 300px;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure 300px is right... The input is supposed to be aligned with the other inputs on the page (except add email/phone, which are wrong).

The common CSS for this is (see .mx_GeneralUserSettingsTab_languageInput):

margin-right: 100px; // Align with the other fields on the page

Copy link
Member Author

Choose a reason for hiding this comment

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

mm - we may have to make all the other fields smaller then, otherwise I don't think we'll have room for the tooltip.

Copy link
Member Author

Choose a reason for hiding this comment

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

done - does this look reasonable?

Copy link
Member

Choose a reason for hiding this comment

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

The tooltip will overextend itself because it's the wrong shape anyways.

I'm not entirely sure the mixin is equivalent either. It looks like the math has gone a bit sideways.

Copy link
Member Author

Choose a reason for hiding this comment

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

Empirically it looks correct to me:

Screenshot 2019-08-13 at 16 22 57

Copy link
Member

Choose a reason for hiding this comment

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

The inputs are supposed to be ~10px from the left edge of the avatar, and clamped there:

image

The inputs are of variable width whereas the "column" the avatar sits in is not.

Copy link
Member

@turt2live turt2live Aug 13, 2019

Choose a reason for hiding this comment

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

also the section texts, labelled toggles, and that random revoke button all need to line up...

edit: I guess it's largely out of scope for this PR at this point, and I can fight the layout if you'd prefer.

Copy link
Member Author

Choose a reason for hiding this comment

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

mm - ok, that's not the design as it appears in the privacy bit though - there the all the fields in all the sections are the same length and are 31px from the profile picture.

Copy link
Member

Choose a reason for hiding this comment

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

it looks like it's using an older version of the settings design. An even still, your screenshot is not 31px :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I see :/ And no, I don't think the tooltip would have fitted with just 31px.

dbkr and others added 4 commits August 13, 2019 10:46
Co-Authored-By: J. Ryan Stinnett <jryans@gmail.com>
Co-Authored-By: J. Ryan Stinnett <jryans@gmail.com>
Co-Authored-By: J. Ryan Stinnett <jryans@gmail.com>
dbkr added 6 commits August 13, 2019 11:05
and make that width narrower so we can fit a tooltip in the left
hand side (they were a little too wide anyway for the kind of data
being entered, even on a narrow window).
as per bug in comment
@dbkr dbkr requested a review from jryans August 13, 2019 14:54
@dbkr
Copy link
Member Author

dbkr commented Aug 13, 2019

sigh, more problems with this:

  • The 3pids and discovery sections need to appear/disappear when you set/clear an ID server
  • The discovery section thinks there are no emails added when the 3pids section . thinks there are

@jryans
Copy link
Collaborator

jryans commented Aug 13, 2019

  • The discovery section thinks there are no emails added when the 3pids section . thinks there are

I spun this out to a separate issue a few days back: element-hq/element-web#10519

@dbkr
Copy link
Member Author

dbkr commented Aug 13, 2019

aha, in that case I fixed the other bug so just potential design issues now.

Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me, assuming some future PR will handle suggesting 3PIDs should be unbound first: element-hq/element-web#10094 (comment).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants