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

Add Gateway Page #628

Merged
merged 11 commits into from
May 3, 2019
Merged

Add Gateway Page #628

merged 11 commits into from
May 3, 2019

Conversation

bafonins
Copy link
Contributor

@bafonins bafonins commented May 3, 2019

Summary

This PR adds the Add Gateway page.
Blocked by: #620
Closes: #621
References: #26

Screenshot 2019-05-03 at 11 37 17

Changes

  • Add the <FrequencyPlansSelect /> component
  • Add the Add Gateway page
  • Update formik
  • Add configuration and user selectors
  • Refactor the Add Device page to use the <FrequencyPlansSelect /> component

cc @htdvisser

@bafonins bafonins added c/console This is related to the Console blocked This can't continue until another issue or pull request is done labels May 3, 2019
@bafonins bafonins added this to the May 2019 milestone May 3, 2019
@bafonins bafonins requested a review from kschiffer May 3, 2019 09:38
@bafonins bafonins self-assigned this May 3, 2019
@coveralls
Copy link

coveralls commented May 3, 2019

Coverage Status

Coverage increased (+0.05%) to 73.591% when pulling 47143ac on feature/621-webui-add-gateway into 2970ec3 on master.

@bafonins bafonins removed the blocked This can't continue until another issue or pull request is done label May 3, 2019
@bafonins bafonins force-pushed the feature/621-webui-add-gateway branch from 068dcad to 59a02b3 Compare May 3, 2019 10:57
@bafonins bafonins requested review from kschiffer and removed request for kschiffer May 3, 2019 10:58
@kschiffer kschiffer mentioned this pull request May 3, 2019
48 tasks
Copy link
Contributor

@kschiffer kschiffer left a comment

Choose a reason for hiding this comment

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

The frequency plan selector in the gateway add form cannot be displayed properly (gets clipped by the viewport):
image
I'm sure there is an option in react-select to show the options above the field.
We now mix strategies a little in terms of connecting to formik, but I think it's ok to do a gradual transition. Formik's connect() look promising indeed.

getGsFrequencyPlans,
} = this.props

if (source === 'ns') {
Copy link
Contributor

Choose a reason for hiding this comment

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

We only need to dispatch if it is not already in the store.

FrequencyPlansSelect.defaultProps = {
title: sharedMessages.frequencyPlan,
autoFocus: false,
horizontal: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
horizontal: true,
horizontal: false,

I know it is not really in line with how we mostly use fields, but it is also false by default in the <Field /> component.

return { userId }
},
dispatch => ({
createSuccess: () => dispatch(push('/console/gateways')),
Copy link
Contributor

Choose a reason for hiding this comment

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

It should redirect to the route of the new gateway.

</Col>
<Col sm={12} md={8}>
<Form
submitEnabledWhenInvalid
Copy link
Contributor

@kschiffer kschiffer May 3, 2019

Choose a reason for hiding this comment

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

While we're at it, can you add this prop also to the application add form to be consistent?

Copy link
Contributor Author

@bafonins bafonins May 3, 2019

Choose a reason for hiding this comment

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

no, will do it in a separate pr

actually, what is the problem with the add application form? it doesnt suffer from the issue we are trying to solve in add device/gateway pages

Copy link
Contributor

Choose a reason for hiding this comment

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

Consistency. We're likely not going to fix this before MVP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what consistency? the add application form has 3 fields and only the first one is required.
again:

it doesnt suffer from the issue we are trying to solve in add device/gateway pages

Copy link
Contributor

Choose a reason for hiding this comment

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

The consistency of the submit button behavior when creating entities. Even better would be to change it throughout all submit buttons, but that's not important enough right now.

// See the License for the specific language governing permissions and
// limitations under the License.

/* eslint-disable import/prefer-default-export */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/* eslint-disable import/prefer-default-export */

Should not be necessary.

@bafonins bafonins force-pushed the feature/621-webui-add-gateway branch from 59a02b3 to 47143ac Compare May 3, 2019 13:57
@bafonins bafonins requested a review from kschiffer May 3, 2019 13:57
Copy link
Contributor

@kschiffer kschiffer left a comment

Choose a reason for hiding this comment

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

LGTM

@bafonins bafonins merged commit ac520a5 into master May 3, 2019
@bafonins bafonins deleted the feature/621-webui-add-gateway branch May 3, 2019 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/console This is related to the Console
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Gateway Page
3 participants