Skip to content
This repository was archived by the owner on Feb 12, 2020. It is now read-only.

Conversation

irina060981
Copy link
Member

For the issue - #323

image

image

@irina060981 irina060981 requested a review from balmas July 30, 2019 09:48
Copy link
Member

@balmas balmas left a comment

Choose a reason for hiding this comment

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

this looks great, just a couple minor ui questions/requests. thanks!

<ui-settings :key="uiSettingsKey" v-show="currentTab === 1"></ui-settings>
<feature-settings :key="featureSettingsKey" v-show="currentTab === 2"></feature-settings>
<resource-settings :key="resourceSettingsKey" v-show="currentTab === 3"></resource-settings>
<div>
Copy link
Member

Choose a reason for hiding this comment

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

Can we put a little divider line or something above the Reset All button to make it clearer that it Resets ALL options, not just the set being viewed? (Alternatively, we could change the functionality to have it reset only the specific set...)

Copy link
Member Author

Choose a reason for hiding this comment

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

I suggest to do it this way:
image

Copy link
Member

Choose a reason for hiding this comment

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

ok, but can we move the button back to the left and put the text in () ? Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Then it would be this way
image

<div class="alpheios-tab-options-switch--item" :class="{ 'alpheios-active': currentTab === 1 }" @click="currentTab = 1">U</div>
<div class="alpheios-tab-options-switch--item" :class="{ 'alpheios-active': currentTab === 2 }" @click="currentTab = 2">F</div>
<div class="alpheios-tab-options-switch--item" :class="{ 'alpheios-active': currentTab === 3 }" @click="currentTab = 3">R</div>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

what do you think about adding a hint just above the U/F/R buttons which says "Select an option set ('U' for user interface options, 'F' for feature options and 'R' for resource options):" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, Bridget! I have forgotten about this!
Fixed.

Copy link
Member

Choose a reason for hiding this comment

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

ok, but if we're going to do it as tooltips instead of a hint, let's change the wording of the toolips to just "User Interface Options", "Feature Options" and "Resource Options". I am not sure if a hint isn't needed still, but let's see what Monica says when she tests. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Bridget, may be I didn't understand you properly ...
What is a hint?
Could you show me an example in some other place on a panel or popup?

Copy link
Member

Choose a reason for hiding this comment

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

well, this is a good point -- because we don't really use hints elsewhere in the interface (and where we've had them, as in the texts interface, Harry keeps asking me to take them out...). So let's just stick with the tooltips for now, rewording just to describe each option as I suggested. Thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

ok - fixed :)

@coveralls
Copy link

coveralls commented Jul 30, 2019

Pull Request Test Coverage Report for Build 3564

  • 5 of 10 (50.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.05%) to 43.382%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/vue/components/options.vue 4 9 44.44%
Totals Coverage Status
Change from base Build 3562: -0.05%
Covered Lines: 1404
Relevant Lines: 3025

💛 - Coveralls

@irina060981
Copy link
Member Author

@balmas , waiting for your final approval :)
I believe I fixed all notes!

Copy link
Member

@balmas balmas left a comment

Choose a reason for hiding this comment

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

Approved!

@irina060981 irina060981 merged commit 50b4437 into master Jul 30, 2019
@irina060981 irina060981 deleted the feat-323-options-tabs branch July 30, 2019 13:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants