Me: My Profile: Update Profile Links to use SectionHeader #808
Conversation
alternatekev
added
[Status] Needs Review
|
Pinging @ebinnion |
| @@ -10,7 +10,8 @@ var FormFieldset = require( 'components/forms/form-fieldset' ), | |||
| FormTextInput = require( 'components/forms/form-text-input' ), | |||
| FormButton = require( 'components/forms/form-button' ), | |||
| eventRecorder = require( 'me/event-recorder' ), | |||
| SimpleNotice = require( 'notices/simple-notice' ); | |||
| SimpleNotice = require( 'notices/simple-notice' ), | |||
|
Functionally, this works well. I left a note about refactoring notices and a couple of style notes. I'll plan on taking a look at this early next week. That should give us some more time to see how the notices PR shapes up. |
| render: function() { | ||
| return( | ||
| <div> | ||
| <SectionHeader label={ this.translate( 'Profile Links' ) }> |
There was a problem hiding this comment.
I had to do this for settings as well, and it made me think that it'd make more sense semantically for SectionHeading to be within the Card instead of having to add an extra div wrapper. What do you think?
There was a problem hiding this comment.
@mtias I've definitely run into areas where having the SectionHeader inside the Card would have helped pass things around without an extra div. I'm all for that update.
There was a problem hiding this comment.
👍 I'll see if we can revisit this before more sections start using it this way.
|
Nice improvement! |
| <Button | ||
| compact | ||
| primary | ||
| className="add-buttons__add-wp-site" |
There was a problem hiding this comment.
The class here should be something like profile-links__add-wp-site or profile-links__add-buttons-wp.
|
Uhm that "Add WordPress" interaction is really odd. I was about to comment about the card-in-card solution, but the issue is even bigger imho: the button "Add a WordPress Site" should open a modal or a different page, not being in-line like it's currently done live. It's an entirely different form, structure and context, and it can get very long. Same oddness for "Add Other Site". A different page would probably be slightly better for me than a modal since that could be linked directly too. Useful for flows and support. :) So I'd say that for the purpose of this PR this is ok, but we should look into refactoring that bit in a different interaction flow in a follow-up, doing a single button "Add" and then there a new page/modal with the two options. |
|
@folletto would you mind opening a new issue outlining your concerns? I think @alternatekev's main focus here was to reuse existing components to bring an old section more inline with our current patterns. When we have more time to do another iteration of /me we can certainly improve things like that. It's definitely important to both track those improvements we'd like to make, but also let this interim improvement get merged. |
|
Yup I can do. I want to see if there was some consensus first. 👍 |
| </FormButton> | ||
| </FormFieldset> | ||
| </form> | ||
| <Card> |
There was a problem hiding this comment.
I'm not sure about the Card within a Card look we generate with this. Do we have examples of that elsewhere?
There was a problem hiding this comment.
If we want to add a division between the two we can drop an hr or add a top/bottom border.
There was a problem hiding this comment.
I tend to agree with @mtias here, I didn't realize the card-within-card was a new addition. I think we could just get away with a bottom border or something on the form.
There was a problem hiding this comment.
@rickybanister @mtias I've removed the extra internal Cards here. @ebinnion: Would you be able to take care of making this work inline with how you did the BackupCodes, where the list of items isn't shown while you're adding one, and dims the buttons in the SectionHeader?
ebinnion
force-pushed
the
update/profile-links-section-header
branch
from
d6fbe0f to
695e850
Compare
@alternatekev - I made the changes as requested. I also went ahead and rebased against master to fix some merge conflicts as well as refactored to simplify some of the markup.
|
|
Nice, this is looking good. @ebinnion mind squashing a few of the commits here? |
Me: ESLint fixes and removal of unused code for profile links
ebinnion
force-pushed
the
update/profile-links-section-header
branch
from
a5894e3 to
32ebb2b
Compare
|
I got rid of 10 commits by squashing @alternatekev's initial 8 commits and my initial 4 commits. If you're fine with my changes @alternatekev, let's |
alternatekev
added
[Status] Ready to Merge
and removed
[Status] Needs Design Review
…header Me: My Profile: Update Profile Links to use SectionHeader
In the process of changing the /me sections to use the SectionHeader pattern, I made the following changes to Profile Links:
It used to look like this:
Now it looks like this:
Clicking "Add WordPress Site" gives you this:
with this rounding things out at the bottom of the page:
Clicking "Add URL" gives you this:
And because I know @rickybanister is going to suggest it: this is essentially an interstitial step because the control buttons shouldn't be at the bottom of the form, they should be in the SectionHeader a la the actual second part of the SectionHeader AddButton pattern. We'll get there in the next PR, along with the Application Password section of Security.