Skip to content

Conversation

@ishanbha
Copy link
Contributor

What changes were proposed in this pull request?

UI changes for the Register/Confirm Hosts step for Mpack work

How was this patch tested?

20392 passing (22s)
125 pending

@asfgit
Copy link

asfgit commented Jan 30, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/338/
Test FAILed.
Test FAILured.

@ishanbha ishanbha requested a review from geksilla January 30, 2018 22:28
@asfgit
Copy link

asfgit commented Jan 30, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/350/
Test FAILed.
Test FAILured.

@asfgit
Copy link

asfgit commented Jan 31, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/352/
Test FAILed.
Test FAILured.

Copy link

Choose a reason for hiding this comment

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

Not a fan of "Rows per page" as it is a bit too literal. "Items" wasn't much better. I would rather see this string be contextual, like "Hosts per page" or whatever the table rows represent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently the way this is implemented is using App.PaginationView which doesn't allow this customization, so this has to be generic (there could be tables not having any hosts but processes or services or components instead). We can of course change the way view is implemented to make it more customizable but that requires a larger effort and the change would need to be reflected in other parts of the code. We can leave it for now, and can re-visit it later if there is a more pressing need.

Copy link

@jgolieb jgolieb Jan 31, 2018

Choose a reason for hiding this comment

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

We really need to standardize the styles around the icons in the Actions columns of tables. That's why I introduced the ".icon-button" and associated styles. Doesn't need to be done in this pull request, but we should restyle these for consistency whenever we come across them. Overly specific style rules like this shouldn't be necessary very often.

Copy link

Choose a reason for hiding this comment

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

Duplicated line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. Got duplicated while merging.

Copy link

Choose a reason for hiding this comment

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

I know that the mockups show specific labels on the next buttons, but I'm not sure it's a great idea. For one thing, it makes it harder to reorder steps if needed. Also, we don't do the same thing to the back buttons. Finally, we have a very prominent navigation menu on the left, so it's not like the user won't know where they're going. If we really want the buttons to be this way, it would be better to make them automatically adjust the labels based on the next/previous steps. I can bring this up with Subhrajit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what to do here. Will wait to hear back from Subhrajit then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it back to next

@ishanbha ishanbha force-pushed the AMBARI-22872-branch-feature-AMBARI-14714-ui branch from c2a0eb1 to be0990f Compare January 31, 2018 18:43
@asfgit
Copy link

asfgit commented Jan 31, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/371/
Test FAILed.
Test FAILured.

@asfgit
Copy link

asfgit commented Jan 31, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/372/
Test FAILed.
Test FAILured.

@asfgit
Copy link

asfgit commented Feb 1, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/377/
Test FAILed.
Test FAILured.

@asfgit
Copy link

asfgit commented Feb 1, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/394/
Test FAILed.
Test FAILured.

@ishanbha ishanbha merged commit 95d5d19 into apache:branch-feature-AMBARI-14714-ui Feb 1, 2018
@asfgit
Copy link

asfgit commented Feb 1, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/395/
Test FAILed.
Test FAILured.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants