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

Org filter via label less helpers #2219

Merged

Conversation

brylie
Copy link
Contributor

@brylie brylie commented Mar 3, 2017

  • Remove unneeded helper
  • add conditional checks for apisCount
  • Decouple API count and 'Filtered by' text.
  • Pluralize i18n strings
  • cleanup
  • consistency between Organization Profile and APIs Catalog

Organization Profile

screenshot_20170303_160454

APIs Catalog

screenshot_20170303_161545

APIs Catalog alternative

screenshot_20170303_160831

@marla-singer
Copy link
Contributor

@brylie Wait feedback from @Nazarah and @bajiat about replace counter
joxi_screenshot_1488548886188

@brylie
Copy link
Contributor Author

brylie commented Mar 3, 2017

We can still work with the text positioning and wording.

I am simply showing that the suggestions will work, since the conversation had become a bit of a struggle. The main point here is to demonstrate the requested changes, suggesting a cleaner way to organize this feature while preserving the user experience. E.g.

screenshot_20170303_160831

@marla-singer
Copy link
Contributor

marla-singer commented Mar 3, 2017

@brylie I don't mind about your suggestion. It allows saving a simple way. But is it okay that we change the design idea?

@brylie
Copy link
Contributor Author

brylie commented Mar 3, 2017

@marla-singer a wireframe/mockup is generally a guide to get started.

When we start implementing the feature, things will naturally change slightly. It is not always possible, or desirable, to implement code exactly as initially sketched - although we try to stick as close to the original intention.

@marla-singer
Copy link
Contributor

@brylie Just recommendation, no obligation?

@brylie
Copy link
Contributor Author

brylie commented Mar 3, 2017

It is not a strict oblication. We try, as developers, to honor the spirit of the design (the intention). However, we sometimes deviate from the design when we encounter limitations or constraints the design did not conceive.

@marla-singer
Copy link
Contributor

Your suggestion is good, anyway. One feature must do one thing. Either show count or filter criterion

@brylie
Copy link
Contributor Author

brylie commented Mar 3, 2017

Cool. I have posted two alternatives for the API Catalog changes (in the PR description). Which do you like better?

@@ -1,11 +1,6 @@
<template name="apiFilteredBy">
{{# if selectedOption }}
<span>
{{# if showApisCount }}
<strong>
{{_ 'apiFilteredBy_text.apiCount' count=apisCount }}&nbsp;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to delete useless i18n tags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't find that tag in the en.i18n.js file.

Copy link
Contributor

Choose a reason for hiding this comment

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

@marla-singer
Copy link
Contributor

The second one. But don't place it in <small> tag, just like in Organization profile

@brylie
Copy link
Contributor Author

brylie commented Mar 3, 2017

Here is what happens when I pull the text outside of the <small></small> tags:

screenshot_20170303_161952

@55
Copy link
Contributor

55 commented Mar 3, 2017

In my opinion, this approach is better.

@brylie
Copy link
Contributor Author

brylie commented Mar 3, 2017

Cool, @Nazarah also just reviewed it and will post her preference.

@marla-singer
Copy link
Contributor

@NNN Sure?
joxi_screenshot_1488551277687
joxi_screenshot_1488551309980

@Nazarah
Copy link
Contributor

Nazarah commented Mar 3, 2017

In catalog view, the number of APIs can appear beside the "APIs" headline.
But "filtered by" text and clear all should come below the sorting bar.
Showing those texts in bigger letters beside headline doesn't make it look good.
Beside two views are different: organization profile and api catalog.
So in catalog, change text about filtered text should appear below the sort/action bar.

@brylie
Copy link
Contributor Author

brylie commented Mar 3, 2017

Thanks @Nazarah. That corresponds with the current code in this branch:
screenshot_20170303_161545

@marla-singer
Copy link
Contributor

marla-singer commented Mar 3, 2017

@brylie clean up i18n tag

@marla-singer marla-singer merged commit ef1525f into feature/org-filter-via-label Mar 3, 2017
@marla-singer marla-singer deleted the org-filter-via-label-less-helpers branch March 3, 2017 14:47
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.

None yet

4 participants