Skip to content

removed some things#13

Open
btopro wants to merge 1 commit into
IST402-Group-F:masterfrom
btopro:master
Open

removed some things#13
btopro wants to merge 1 commit into
IST402-Group-F:masterfrom
btopro:master

Conversation

@btopro

@btopro btopro commented Nov 9, 2021

Copy link
Copy Markdown

20/20
Overview:
Nice work getting chain selectors working to do deep querying in the shadowRoot's for automated testing
and reasonable tests (5 of them). The stories look good and you did all of them so +1 there. Initial look
at the card is visually clean and supports 3 different types that match the comp (purple might be diff color but meh).

topText and bottomText can't be changed by the user which is unfortunate and is not translatable or able to
be changed via slots. <h1 slot="top-header">${this.topText}</h1> in learning-banner does nothing for example
as you'd need <h1><slot name="top-header">.... as well as compositing for it to pass down. You do get compositing
correct elsewhere but shoot for users being able to override the implementation to set slot="..." in the demo /
top level element.

Overall looks correct, nothing fancy but gets the job done and very little code to remove. Also watch for
blank ...super.properties statements when there's nothing being extending as well as empty methods that
just call super.whatever.

PR:


Detail breakdown:
-.5 for no named slots at top level for implementing in design
-.5 properties can't update the title headings
+1 for stories for all elements correctly

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.

1 participant