Component: ebay-notice #28

sharma46bhawana · 2018-03-07T02:00:20Z

Description

Contains a new component ebay-notice which constructs page and inline alerts.

Context

As the alerts can be page or inline alerts (see ebay/skin) we want to get that as an input for customisation. Default is set to page when no input is provided.
For a page alert we have a heading tag in the icon that provides it is ordering with a11y users. To provide that customisation we take the heading-tag as an input for customisation. Default is set to h2 when no input is provided.
As the alerts are differentiated into 3 types: confirmation, priority and information, we take the type as an input for the customisation. Default is set to priority when no input is provided.

References

This will complete issue #5

Screenshots

Page notice with type: priority

Inline notice with type: confirmation

coveralls · 2018-03-07T02:05:35Z

Pull Request Test Coverage Report for Build 73

9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.4%) to 87.665%

Totals
Change from base Build 66:	0.4%
Covered Lines:	392
Relevant Lines:	434

💛 - Coveralls

ianmcburnie · 2018-03-07T02:58:49Z

package.json

@@ -43,7 +43,7 @@
    "marko-components"
  ],
  "devDependencies": {
-    "@ebay/skin": "^3.3.0-0",
+    "@ebay/skin": "^3.4.0-0",


You can change this to 3.4.0 now. We have verified it (don't forget to regenerate yarn.lock). Or you can simply rebase and force push my commit from 0.1.0 branch. Up to you. Either way should resolve the conflicts below.

okay I will rebase from 0.1.0

ianmcburnie · 2018-03-07T03:19:52Z

.gitignore

@@ -12,3 +12,5 @@ dist
 .browser-refresh
 integration/template.marko
 integration/browser.json
+/.vscode


We can let these unrelated changes slip through this time, but team, I'd like us to get into the habit of creating separate PRs for anything unrelated to the main issue at hand. This may seem picky, but imagine if we have to rollback this feature commit (for whatever reason) after it is squashed, we probably wouldn't want to roll back these useful .gitignore updates too, but that's what would happen (and then we'd have to re-commit them again). The upgrade of skin dependency is another example of this. I know I'm being exceptionally picky in this case. Sorry ;-)

I can create a different PR for this, I am not sure about creating an issue though

Let me just do that

No need. Seriously. I was just using this as an example :-)

I'm 100% in agreement with this. I know I just try to get improvements in, but small PRs for minor improvements are actually good, too.

Although, I do know there are times when I don't want to forget! So, I guess we create an issue to track it, @ianmcburnie?

seangates · 2018-03-07T02:39:45Z

src/components/ebay-notice/examples/1-page-notice/template.marko

+        level="page"
+        aria-label-text="this is a notice">
+        <p><strong>Error:</strong> Please take another look at the following:
+        </p>


Small suggestion, should be all on the same line:

Error: Please take another look at the following:

OR, on different lines like paragraph below it.

 Error: Please take another look at the following: 

same as below

seangates · 2018-03-07T02:40:13Z

src/components/ebay-notice/examples/2-inline-notice/template.marko

+        level="inline"
+        aria-label-text="this is a notice">
+        <p> Couldn't load all the items, please try again later.
+        </p>


Same comment about whitespace here.

Yes that looks ugly, I will fix it

seangates · 2018-03-07T02:51:01Z

src/components/ebay-notice/index.js

+    model.type = state.type || defaults.type;
+    const level = model.level;
+    const headingTag = (level === 'page') ?
+        (state.headingTag || constants[level].headingTag) : constants[level].headingTag;


MINOR: Maybe We should just reset the state.headingTag beforehand rather than having it with the ||. Like an intermediary const?

const level = state.level || defaults.level; const type = state.type || defaults.type; let headingTag = state.headingTag || constants[level].headingTag; if (level !== 'page') { headingTag = constants[level].headingTag; }

Just an exploration, to make the logic pretty clear for the next developer who will look at the code of this component.

hmmm now that I am thinking about it I can do
const headingTag = (level === 'page' && state.headingTag) ? state.headingTag : constants[level].headingTag;

That works! Much easier to understand now.

seangates · 2018-03-07T03:02:19Z

src/components/ebay-notice/index.js

+    model.renderBody = state.renderBody;
+    model.class = state.class || '';
+
+    return model;


Both the other components (menu, listbox) just use an object returned:

return { type: state.type, isRadio: state.isRadio, isCheckbox: state.isCheckbox, isNotCheckable: !state.isRadio && !state.isCheckbox, label: state.label, expanded: state.expanded, menuClass: menuClass, buttonClass: buttonClass, itemsClass: itemsClass, role: !state.fake ? 'menu' : null, items: state.items, htmlAttributes: state.htmlAttributes };

@ianmcburnie @yomed Should we enforce a consistent style for code like this, in the lifecycle methods?

okay I can make the changes

seangates · 2018-03-07T03:32:05Z

src/components/ebay-notice/template.marko

+<${data.mainTag}
+    w-bind
+    aria-labelledby="${data.type}-status-${widget.elId()}"
+    class="${data.level}-notice ${data.level}-notice--${data.type} ${data.class}"


To be consistent with the other components, the class creation/management should be moved to index.js in the getTemplateData method.

I was thinking whether I should keep it in index.js when I sent a PR. Okay I will do it

For the case of notice I would have to create 3 data classes as all three DOM elements would have different classes according to the input

Maybe just experiment with how you might add classes. Here are two examples:

Menu
https://github.com/eBay/ebayui-core/blob/master/src/components/ebay-menu/index.js#L84-L96

Listbox (soon to be Select)
https://github.com/eBay/ebayui-core/blob/master/src/components/ebay-listbox/index.js#L48-L50

I'm sure you can come up with something to satisfy creating the classes. Also, remember that you can use arrays which make adding the class names very easy.

Okay I will make the changes, thanks! :)

seangates · 2018-03-07T03:32:14Z

src/components/ebay-notice/index.js

+const processHtmlAttributes = require('../../common/html-attributes');
+const template = require('./template.marko');
+
+


Remove this extra line. 😁

As a side note, we may want to enforce this through eslint:
https://eslint.org/docs/rules/no-multiple-empty-lines

I would approve of that. 👍

seangates · 2018-03-07T03:36:16Z

src/components/ebay-notice/marko-tag.json

@@ -0,0 +1,8 @@
+{


I believe this file (marko-tag.json) and the marko.json won't work in Marko v4, and that configuration needs to be moved up into the root marko.json of the project, per #15.

Therefore, please move this configuration up into the root ./marko.json.

Oh I missed it, my bad. Will correct it.

senthilp · 2018-03-07T05:09:56Z

src/components/ebay-notice/index.js

+    return input;
+}
+function getTemplateData(state) {
+    const model = {};


@sharma46bhawana You could also do

const model = Object.assign({}, defaults, state);

and avoid

model.level = state.level || defaults.level; model.type = state.type || defaults.type;

BTW, the above two lines are repeated twice. Maybe a typo.

I think I don't need model anymore

Right, you don't need model any longer.

yomed

Looks solid overall, just some minor comments.

yomed · 2018-03-07T17:18:23Z

src/components/ebay-notice/README.md

+### `ebay-notice` Usage
+
+```marko
+<ebay-notice heading-tag="h3" type="priority" level="page" aria-label-text="this is a notice">


(minor) Would "this is a notice" be used in real world usage of this component? If not, maybe we can improve it to approximate practical usage (across all examples that use this text).

Right, I will change it to @ianmcburnie's suggestion

yomed · 2018-03-07T17:23:21Z

src/components/ebay-notice/index.js

+    level: 'page',
+    type: 'confirmation'
+};
+function getInitialState(input) {


You should be able to remove this function entirely -- the default Marko behavior already returns input.

Okay, I thought that as its in compatibility layer it might still need the getInitialState, if not i will remove it :)

ianmcburnie · 2018-03-07T16:57:26Z

src/components/ebay-notice/examples/2-inline-notice/template.marko

+    <ebay-notice
+        type="confirmation"
+        level="inline"
+        aria-label-text="this is a notice">


Please change all example aria-label-text to one of "Confirmation", "Priority" or "Information".

Okay will do it.

ianmcburnie · 2018-03-07T16:58:02Z

src/components/ebay-notice/README.md

+`level` | String | No | "inline" or "page"
+`type`  | String | No | "priority", "confirmation" or "information"
+`aria-label-text` | String | No | adding description for the notice for a11y users
+`heading-tag` | String | No| used in case of "page" level notices to specify the heading tag according to the notice's placement


I suggest we use heading-level, with a value of 1 to 6. This actually falls in line nicely with aria-level.

I actually like that. I will make the changes

ianmcburnie · 2018-03-07T17:58:35Z

I suggest we rename level to type, and type to status. I am open to other suggestions for these names too.

seangates · 2018-03-07T20:49:59Z

You should work with @ianmcburnie on the last bit, though.

seangates · 2018-03-07T20:50:15Z

Oh, and there are now conflicts, it appears.

sharma46bhawana · 2018-03-07T22:14:17Z

@ianmcburnie made the changes to make the names, @yomed made the changes you suggested. Please take a look when you get time

yomed · 2018-03-08T01:27:04Z

src/components/ebay-notice/README.md

+`class` | String | No | custom class
+`type` | String | No | "inline" or "page"
+`status`  | String | No | "priority", "confirmation" or "information"
+`aria-level` | String | No | adding description for the notice for a11y users


@ianmcburnie Did you intend to have this called aria-level rather than something like aria-text or aria-label-text?

No, my intention was not for it to be aria-level. Apologies if I caused confusion. Maybe heading-text? Or icon-aria-label - whatever would be consistent with other components.

I think aria-text would be a better.

Okay, let's go with aria-text. This text is also used to label the region/section (via aria-labelledby) - so I think this choice is a good all round fit.

yomed · 2018-03-08T18:22:40Z

src/components/ebay-notice/README.md

+### `ebay-notice` Usage
+
+```marko
+<ebay-notice heading-level="3" status="priority" type="page" aria-level="Priority">


Make sure to update the aria-level reference here

yes will do it now

yomed · 2018-03-08T18:23:15Z

src/components/ebay-notice/index.js

+        renderBody: input.renderBody,
+        mainClass: [`${type}-notice`, `${type}-notice--${status}`, input.class],
+        headingClass: [`${type}-notice__status`],
+        contentClass: [`${type}-notice__content`]


headingClass and contentClass can be regular strings instead of arrays.

okay makes sense

ianmcburnie

Nice job Bhawana!

bhawsharma added 2 commits March 6, 2018 17:23

Added ebay-notice component

17f0b31

Added the ebay-notice folder and added entry in the main README

ae7bedf

sharma46bhawana requested review from ianmcburnie, seangates, senthilp and yomed March 7, 2018 02:17

ianmcburnie reviewed Mar 7, 2018

View reviewed changes

Rebasing from 0.1.0

7a12eca

sharma46bhawana force-pushed the 5 branch from 64237ed to 7a12eca Compare March 7, 2018 03:18

ianmcburnie reviewed Mar 7, 2018

View reviewed changes

seangates reviewed Mar 7, 2018

View reviewed changes

sharma46bhawana added the component: notice label Mar 7, 2018

seangates suggested changes Mar 7, 2018

View reviewed changes

senthilp reviewed Mar 7, 2018

View reviewed changes

Addressing PR comments

14d2420

sharma46bhawana force-pushed the 5 branch from 8cb4cf7 to 14d2420 Compare March 7, 2018 05:29

yomed reviewed Mar 7, 2018

View reviewed changes

ianmcburnie requested changes Mar 7, 2018

View reviewed changes

seangates approved these changes Mar 7, 2018

View reviewed changes

Resolving conflicts

4d98a38

sharma46bhawana closed this Mar 7, 2018

sharma46bhawana reopened this Mar 7, 2018

sharma46bhawana force-pushed the 5 branch 2 times, most recently from 36ceaa0 to f9d3ef8 Compare March 7, 2018 23:29

sharma46bhawana force-pushed the 5 branch 2 times, most recently from 65361ff to 3c54a29 Compare March 7, 2018 23:32

yomed reviewed Mar 8, 2018

View reviewed changes

sharma46bhawana force-pushed the 5 branch 2 times, most recently from 5327982 to 08e61bf Compare March 8, 2018 18:19

yomed approved these changes Mar 8, 2018

View reviewed changes

Addressing PR comments

b5e175b

sharma46bhawana force-pushed the 5 branch from 08e61bf to b5e175b Compare March 8, 2018 18:29

ianmcburnie approved these changes Mar 8, 2018

View reviewed changes

ianmcburnie merged commit 48948ba into 0.1.0 Mar 8, 2018

ianmcburnie deleted the 5 branch March 8, 2018 18:55

jammur added the component: notice label Jun 14, 2018

		const processHtmlAttributes = require('../../common/html-attributes');
		const template = require('./template.marko');

Component: ebay-notice #28

Component: ebay-notice #28

Conversation

sharma46bhawana commented Mar 7, 2018 • edited

Description

Context

References

Screenshots

coveralls commented Mar 7, 2018 • edited

Pull Request Test Coverage Report for Build 73

💛 - Coveralls

ianmcburnie Mar 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharma46bhawana Mar 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yomed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianmcburnie commented Mar 7, 2018

seangates commented Mar 7, 2018

seangates commented Mar 7, 2018

sharma46bhawana commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianmcburnie left a comment

Choose a reason for hiding this comment

sharma46bhawana commented Mar 7, 2018 •

edited

coveralls commented Mar 7, 2018 •

edited

ianmcburnie Mar 7, 2018 •

edited

sharma46bhawana Mar 7, 2018 •

edited