Adding updated Server Tab which shows common server variables (for #674) #712

Merged
merged 2 commits into from Feb 3, 2014

Conversation

Projects
None yet
3 participants
@bryanjhogan
Contributor

bryanjhogan commented Jan 24, 2014

See issue - #674

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Jan 30, 2014

Member

For the record is it possible to get a list of the major additions here and maybe a screen shot?

Member

avanderhoorn commented Jan 30, 2014

For the record is it possible to get a list of the major additions here and maybe a screen shot?

@bryanjhogan

This comment has been minimized.

Show comment
Hide comment
@bryanjhogan

bryanjhogan Jan 30, 2014

Contributor

The server tab has been returned to the list of tabs. The servers variables are now split in rough groupings - Http Variables, Other Variables, Cert Variables, and Https Variables.

Screen shot 1
servertab01

Screen shot 2

servertab02

Contributor

bryanjhogan commented Jan 30, 2014

The server tab has been returned to the list of tabs. The servers variables are now split in rough groupings - Http Variables, Other Variables, Cert Variables, and Https Variables.

Screen shot 1
servertab01

Screen shot 2

servertab02

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Jan 30, 2014

Collaborator

@bryanjhogan maybe a small side-note, but I would suggest to rearrange the different groups, like first the HttpVariables followed by the HttpsVariables, the the CertVariables and I would put the Other variables at the end, otherwise if they are second, you would think that they contain all other variables and when you scroll further you see an additional two groups

what do you think?

Collaborator

CGijbels commented Jan 30, 2014

@bryanjhogan maybe a small side-note, but I would suggest to rearrange the different groups, like first the HttpVariables followed by the HttpsVariables, the the CertVariables and I would put the Other variables at the end, otherwise if they are second, you would think that they contain all other variables and when you scroll further you see an additional two groups

what do you think?

@bryanjhogan

This comment has been minimized.

Show comment
Hide comment
@bryanjhogan

bryanjhogan Jan 30, 2014

Contributor

@CGijbels I had them in that order, but I do most of my development without ssl, so Cert and Https will be empty. I felt that having the ones with useful information at the top was better.

But I see your point. Maybe if it was called something other than "Other"?

Contributor

bryanjhogan commented Jan 30, 2014

@CGijbels I had them in that order, but I do most of my development without ssl, so Cert and Https will be empty. I felt that having the ones with useful information at the top was better.

But I see your point. Maybe if it was called something other than "Other"?

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Jan 30, 2014

Collaborator

@bryanjhogan When you said so Cert and Https will be empty do you mean there are no keys for them or just empty values? Because if there are no keys, then you could prevent that group from appearing altogether, if it's only empty values then I understand your point of bringing the Others after the Http ones.

Maybe instead of Others name then General variables or Remaining non-security related variables ? And group the CERT and HTTPS variables under a group Security related variables ?

Just thinking out loud, no obligation here whatsoever

what do you guys think?

Collaborator

CGijbels commented Jan 30, 2014

@bryanjhogan When you said so Cert and Https will be empty do you mean there are no keys for them or just empty values? Because if there are no keys, then you could prevent that group from appearing altogether, if it's only empty values then I understand your point of bringing the Others after the Http ones.

Maybe instead of Others name then General variables or Remaining non-security related variables ? And group the CERT and HTTPS variables under a group Security related variables ?

Just thinking out loud, no obligation here whatsoever

what do you guys think?

@bryanjhogan

This comment has been minimized.

Show comment
Hide comment
@bryanjhogan

bryanjhogan Jan 31, 2014

Contributor

@CGijbels What I meant was if you are not using an ssl connection all the values for Cert and HTTPS will be empty.

I like the idea of Http, General, and grouping cert and https into Security related variables.

Here a rough copy paste of how it looks, ignore the repeated Firefox status bar :)

servertablong

Contributor

bryanjhogan commented Jan 31, 2014

@CGijbels What I meant was if you are not using an ssl connection all the values for Cert and HTTPS will be empty.

I like the idea of Http, General, and grouping cert and https into Security related variables.

Here a rough copy paste of how it looks, ignore the repeated Firefox status bar :)

servertablong

@bryanjhogan

This comment has been minimized.

Show comment
Hide comment
@bryanjhogan

bryanjhogan Jan 31, 2014

Contributor

Another possible layout, but it looks a bit ugly to me.

servertab

Contributor

bryanjhogan commented Jan 31, 2014

Another possible layout, but it looks a bit ugly to me.

servertab

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Jan 31, 2014

Member

I don't think I like the latest version as much as the one where you have the XYZ Variables headings. Later on, if we get more information in this tab, we can make changes to the layout, but for the moment I think this is best.

Member

avanderhoorn commented Jan 31, 2014

I don't think I like the latest version as much as the one where you have the XYZ Variables headings. Later on, if we get more information in this tab, we can make changes to the layout, but for the moment I think this is best.

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Jan 31, 2014

Collaborator

@avanderhoorn so to be clear, @bryanjhogan should go/keep this version (for completeness: that's the one I like the most as well)?

Collaborator

CGijbels commented Jan 31, 2014

@avanderhoorn so to be clear, @bryanjhogan should go/keep this version (for completeness: that's the one I like the most as well)?

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Feb 1, 2014

Member

Yep, that's my thinking. Thanks for linking to it so we are all on the same
page.

On Friday, January 31, 2014, Christophe Gijbels notifications@github.com
wrote:

@avanderhoorn https://github.com/avanderhoorn so to be clear,
@bryanjhogan https://github.com/bryanjhogan should go/keep this versionhttps://github.com/Glimpse/Glimpse/pull/712#issuecomment-33807605(for completeness: that's the one I like the most as well)?

Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/712#issuecomment-33853015
.

Member

avanderhoorn commented Feb 1, 2014

Yep, that's my thinking. Thanks for linking to it so we are all on the same
page.

On Friday, January 31, 2014, Christophe Gijbels notifications@github.com
wrote:

@avanderhoorn https://github.com/avanderhoorn so to be clear,
@bryanjhogan https://github.com/bryanjhogan should go/keep this versionhttps://github.com/Glimpse/Glimpse/pull/712#issuecomment-33807605(for completeness: that's the one I like the most as well)?

Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/712#issuecomment-33853015
.

@bryanjhogan

This comment has been minimized.

Show comment
Hide comment
@bryanjhogan

bryanjhogan Feb 2, 2014

Contributor

Will update the code in PR in a few hours.

Contributor

bryanjhogan commented Feb 2, 2014

Will update the code in PR in a few hours.

avanderhoorn added a commit that referenced this pull request Feb 3, 2014

Merge pull request #712 from bryanjhogan/AddServerTab
Adding Server tab, ServerModel and ServerShould

@avanderhoorn avanderhoorn merged commit 6bf24e5 into Glimpse:master Feb 3, 2014

@avanderhoorn avanderhoorn added this to the vNext milestone Feb 3, 2014

@avanderhoorn avanderhoorn self-assigned this Feb 3, 2014

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