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

Adding status field in Domain's Summary page's properties section. #2223

Merged
merged 5 commits into from Oct 5, 2017

Conversation

xeviknal
Copy link
Member

@xeviknal xeviknal commented Sep 25, 2017

This PR is related to #44. It only adds a new property field into Domain's summary page.

Domain's summary page:
adding-status-domain

Here the addition:
domain-properties-table

@karelhala
Copy link
Contributor

Pending PR in schema is ManageIQ/manageiq-schema#72
@miq-bot add_label middleware, enhancement

@abonas
Copy link
Member

abonas commented Sep 26, 2017

@dclarizio why is the fine/no flag needed now?

@abonas
Copy link
Member

abonas commented Sep 26, 2017

@xeviknal for UI related changes, a screenshot is required in the first comment in the PR so the reviewers (and especially those who are not in the middleware team) can easily understand what's that about. So, please add a screenshot.

@@ -9,6 +9,10 @@ def textual_nativeid
@record.nativeid
end

def textual_status
@record.status
Copy link
Member

@abonas abonas Sep 26, 2017

Choose a reason for hiding this comment

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

I think translation is needed, similar to the below - how tags are handled.
@mtho11 @karelhala is that indeed so?

@abonas
Copy link
Member

abonas commented Sep 26, 2017

@karelhala @mtho11 please review

@xeviknal
Copy link
Member Author

@abonas added the screenshots

@dclarizio
Copy link

@abonas I normally set enhancements to not be backported . . . feel free to change or remove the flag if you know better. 😄

@abonas
Copy link
Member

abonas commented Sep 26, 2017

@dclarizio I'm with you on this :) , but we are not during that time of release, and I do not see anyone in any other repo setting those flags, hence I wondered.

@dclarizio
Copy link

@abonas thanks . . . I figure I'll make my best guess and I try to fill in at least one purple label along with any version labels I can when I first touch the PRs. I just don't like non-labeled content. :)

@dclarizio dclarizio self-assigned this Sep 28, 2017
@xeviknal
Copy link
Member Author

xeviknal commented Oct 4, 2017

@karelhala and @dclarizio could you please check I am not messing anything in here, please?

@dclarizio
Copy link

@xeviknal just wondering if you think the status should be titleized, "running" becomes "Running".

@xeviknal
Copy link
Member Author

xeviknal commented Oct 5, 2017

@dclarizio I see most of the statuses are titleized (capitalized). Althought I have just seen deploys/servers' statuses are not translated, they are printed as they come from database.

Then 2 thing:

  • Should I apply translation to other statues ( @karelhala I thing we talked about that)
  • Should I apply .titleize to all 3 statuses?

From my point of view, I would do both of them.

@dclarizio
Copy link

@xeviknal I'd do them both, I just think it looks nicer. Up to you if that should be done in a separate PR.

@xeviknal
Copy link
Member Author

xeviknal commented Oct 5, 2017

@dclarizio I was thinking exactly the same. Probably, I would do that in another PR. In order no to mix purposes.

@miq-bot
Copy link
Member

miq-bot commented Oct 5, 2017

Checked commits xeviknal/manageiq-ui-classic@64b1ec8~...0d06d3f with ruby 2.3.5, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@dclarizio dclarizio merged commit 4d52377 into ManageIQ:master Oct 5, 2017
@dclarizio dclarizio added this to the Sprint 71 Ending Oct 16, 2017 milestone Oct 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants