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

Edit Entry Details in Management Interface #90

Merged
merged 1 commit into from Feb 28, 2019

Conversation

@lmorchard
Copy link
Contributor

commented Feb 25, 2019

  • Tweaks to editing fields to better match UX spec
  • Item title now appears in header for editing
  • Disabled origin field on editing
  • Add delete button in header of edit view
  • New delete.svg icon

Fixes #34

@lmorchard lmorchard requested a review from mozilla-lockwise/desktop-engineering as a code owner Feb 25, 2019

@ghost ghost assigned lmorchard Feb 25, 2019

@ghost ghost added the in progress label Feb 25, 2019

@linuxwolf
Copy link
Member

left a comment

Look and behaves awesomely! This just about matches the designs 🎉 .

Inline there are a requested change or two. The biggest thing I've seem missing is a callout "bubble" when the "website address" field is focused, with instructions on what's expected:
image

Also, I've encountered an interesting bug: a very long password (not entirely sure on the size, sorry!)
will not display the last several characters when revealed from the "Edit" state.

// Focus the first editable field...
if (this.props.fields.itemId) {
// For existing items, the first field is disabled.
this._secondField.focus();

This comment has been minimized.

Copy link
@linuxwolf

linuxwolf Feb 26, 2019

Member

I think there's been some change in product requirements here (i.e., "Website address" is always editable). cc @sandysage for confirmation.

If so, I would propose a follow-up issue; there be dragons if this field is editable.

This comment has been minimized.

Copy link
@linuxwolf

linuxwolf Feb 27, 2019

Member

discussed with @sandysage , and there is a change that had not been reflected into the desktop designs yet (it is in the mobile CRUD designs).

I'm going to file a follow up for this; editable hostname has complications/concerns we need to make sure we understand.

<h1>nEw iTEm</h1>
</Localized>
) : (
<React.Fragment>

This comment has been minimized.

Copy link
@linuxwolf

linuxwolf Feb 26, 2019

Member

TIL about React.Fragment 🌈 🌟

This comment has been minimized.

Copy link
@lmorchard

lmorchard Feb 26, 2019

Author Contributor

Yeah, I think you can also use <> and </> but I like explicitly naming the React.Fragment component.

position: relative;
}

.item-details > h1,

This comment has been minimized.

Copy link
@linuxwolf

linuxwolf Feb 26, 2019

Member

will this conflict with identical work in #87 ?

This comment has been minimized.

Copy link
@lmorchard

lmorchard Feb 26, 2019

Author Contributor

Hmm, it probably will, but we should probably also just rebase / revise against whichever lands first? Not sure how to pre-emptively handle that well

src/list/components/item-fields.js Outdated Show resolved Hide resolved
@lmorchard

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

The biggest thing I've seem missing is a callout "bubble" when the "website address" field is focused, with instructions on what's expected

I think that's part of issue #30 as the "helper" in the specs. At least, that's where I started implementing it on a branch a few weeks ago

Also, I've encountered an interesting bug: a very long password (not entirely sure on the size, sorry!)
will not display the last several characters when revealed from the "Edit" state.

Hmm, probably have some text disappearing under the hide / show button, will take a look

@linuxwolf

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

I think that's part of issue #30 as the "helper" in the specs. At least, that's where I started implementing it on a branch a few weeks ago

Ah, right. Ignore this and carry on!

@lmorchard lmorchard force-pushed the lmorchard:34-edit-details branch 2 times, most recently from bc9b64a to 0abb5f1 Feb 26, 2019

@lmorchard

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

Okay, made some tweaks to fix the hidden bit of the password field and updated the field placeholder. Not entirely sure how / if to be proactive about a conflict with #87

@lmorchard

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

Took a look at #87 - commented to the effect that maybe we should land the other PR first and then I can revise this one to fix the conflicts

@linuxwolf

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Just checked it over, and it's working great! At worst, the cursor disappears at the extreme edge, but arrowing left brings it back to visible.

Edit Entry Details in Management Interface
- Tweaks to editing fields to better match UX spec
- Item title now appears in header for editing
- Disabled origin field on editing
- Add delete button in header of edit view
- New delete.svg icon

Fixes #34

@lmorchard lmorchard force-pushed the lmorchard:34-edit-details branch from 0abb5f1 to 7ed84c8 Feb 28, 2019

@lmorchard

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

Rebased, resolved conflicts. Mainly little CSS tweaks, so I might merge after a successful test run.

@lmorchard lmorchard merged commit cf9edbd into mozilla-lockwise:master Feb 28, 2019

4 checks passed

WIP Ready for review
Details
codecov/patch 71.4% of diff hit (target 50%)
Details
codecov/project 95% (-0.2%) compared to 763f7dc
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ghost ghost removed the in progress label Feb 28, 2019

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