-
Notifications
You must be signed in to change notification settings - Fork 2k
Hosting Dashboard: Add domain glue records add/edit screen #105213
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
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
| // eslint-disable-next-line no-restricted-imports | ||
| import { ValidatedInputControl } from '@automattic/components/src/validated-form-controls/components/input-control'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the ValidatedInputControl is still in beta, so I'm not sure if this is the right thing to do.
I'm using this because I found no other component that has a validated input that can have a suffix.
@hambai, I saw that you faced the same issue in #105113. Do you think this is an ok approach for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that what this message was referring to?
If it is something we need for domain input boxes, maybe this is something we could create and contribute upstream 🤔 A place to ask about it might be #atelier-data-views
We could leave the suffix out of the component for now and follow up once we have a done the component work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can import it directly...so I didn't use it in #105191
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping! Those validation components are kind of meant to be a temporary solution, especially now that we're working on a new set of components. Hence the reluctance to make them directly available to consumers.
I also just recently removed them from the wp-calypso repo entirely (#105219), since canonical development has already moved to the Gutenberg repo.
At the moment we're exposing them for external use mainly through the DataForm abstraction in @wordpress/dataviews. Is that an option here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Not sure how far DataForm wants to support extension points like this in the standard data types.
I can see how it'd be beneficial to support this, so consumers can configure the Edit component.
My main concern is that we shouldn't couple DataForm to a specific component implementation. We want to be future-proof, which means that we can update the underlying components when/if require it. I don't think we should allow passing any prop to the underlying components, for example. This needs to be discussed in a case-by-case basis.
@mirka do you think prefix/suffix for inputs is a common enough concept that we can incorporate it to DataForm and be future-proof?
I'm thinking we could do this via the Edit function of a field. It's now a React component or a string (e.g., text, email, integer). It could also be an object with some props to be passed down to the component. For example:
{
id: 'field',
Edit: {
type: 'text',
suffix: 'suffix-will-go-here'
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally understand how you wouldn't want to couple DataForm to underlying component implementations, so personally I'm not pushing too hard in that direction.
And the validated form components will be available for public use eventually, in one form or another, so in the long run it's not absolutely necessary that DataForm support these customizations in the standard field types.
On the future-proof angle, the prefix/suffix props themselves are pretty stable, and there are no plans to remove them even in the next-gen components. However, there's a subcomponent (InputControlPrefixWrapper) that can be used to control padding in the prefix/suffix slots, and that part is subject to change 😅 Plus those old subcomponents will not work with the next-gen InputControls. So these types of small back compat issues cannot be 100% avoided if we go down this route. If this is the first time this request has come up, it's maybe too early to say that this is a common enough use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with temporarily exposing any of the validated form components you need as an unlocked API in @automattic/components
@mirka, just to make sure I understand, our option right now is to unlock ValidatedInputControl and wait for the validated form components to become public. Is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thank you!
| // eslint-disable-next-line no-restricted-imports | ||
| import { ValidatedInputControl } from '@automattic/components/src/validated-form-controls/components/input-control'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that what this message was referring to?
If it is something we need for domain input boxes, maybe this is something we could create and contribute upstream 🤔 A place to ask about it might be #atelier-data-views
We could leave the suffix out of the component for now and follow up once we have a done the component work?
fushar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we rename domains/overview-glue-records to domains/domain-glue-records, to be consistent with the others?
p-jackson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some more feedback but after that I think the code is looking good from my point of view.
I think when it comes to that input field which includes the subdomain it would be better to just leave it like Arthur did in the earlier PR. https://app.graphite.dev/github/pr/Automattic/wp-calypso/105213/Hosting-Dashboard-Add-domain-glue-records-add%2Fedit-screen#comment-PRRC_kwDOArzw_86ICrFp
It seems like we should figure it out upstream and then we can bulk-fix-up the Hosting Dashboard input fields that need it.
18b30e3 to
2f59398
Compare
Thanks for the review, @fushar. The folder was renamed. 👍
@p-jackson Makes sense. I think I've addressed all the feedback. @p-jackson @arthur791004 @fushar, could you please give it another look? 🙏 |
This allows for adding the needed suffix.
The `ValidatedInputControl` component is still in beta, so we decided not to use it. This means that the input won't show error messages for now.
76dee31 to
1a57c05
Compare
leonardost
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and LGTM! I think the code looks great. I have some nitpicks but none are blockers, I think we can deploy this and iterate.
Maybe we could have a "Cancel" button to go back to the glue records list page?
This list shouldn't be too long (domains who use glue records usually have 2 or 3), so I wonder if we need to show the filtering and display options here.
The suffix part in this input looks weird for long domain names, but I guess we'll change these components soon.
Sounds good. Thanks for the review, Leo! |
|
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/19980504 Some locales (Brazilian Portuguese, Hebrew) have been temporarily machine-translated due to translator availability. All other translations are usually ready within a few days. Untranslated and machine-translated strings will be sent for translation next Monday and are expected to be completed by the following Friday. Thank you @m1r0 for including a screenshot in the description! This is really helpful for our translators. |
| isValid: { | ||
| required: true, | ||
| custom: ( item ) => { | ||
| if ( ! isValidNameServerSubdomain( item.nameServer ) ) { | ||
| return __( 'Please enter a valid name server.' ); | ||
| } | ||
| return null; | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, I just want to make sure we are aware that the rules provided here are not doing anything because they're not handled in the custom Edit control.
Edit: see how it's implemented in the input control for DataForm and the custom Edit example in the storybook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, André. I'll refactor this part in a future PR.
Part of DOTDASH-202
Proposed Changes
dv9Srg.mov
Why are these changes being made?
Testing Instructions
/v2/domains/[domain]/glue-recordsand clickAdd glue record.Pre-merge Checklist