Skip to content

[angular-ngrx-scss] - Gists panel improvements#456

Merged
SDaian merged 7 commits intomainfrom
angular-ngrx-scss/gists-panel-improvements
Jul 12, 2022
Merged

[angular-ngrx-scss] - Gists panel improvements#456
SDaian merged 7 commits intomainfrom
angular-ngrx-scss/gists-panel-improvements

Conversation

@SDaian
Copy link
Contributor

@SDaian SDaian commented Jul 11, 2022

Description

  • accessibility: Consider using semantic HTML elements ul and li here.
  • styling: needs mobile styling (gist section should move to bottom of page on small screens, see next-react-query deployed app for example)

Closes #186

@SDaian SDaian self-assigned this Jul 11, 2022
@SDaian SDaian added this to the Angular + NgRx + SCSS milestone Jul 11, 2022
@aws-amplify-us-east-1
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-456.dbm2zhgk5abrj.amplifyapp.com

@aws-amplify-us-east-1
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-456.d1190gmno2hljg.amplifyapp.com

@aws-amplify-us-east-1
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-456.d3sqkonra6x7y9.amplifyapp.com

@SDaian SDaian requested review from Amdrel and lindakatcodes July 11, 2022 17:07
padding: 1rem;
border-bottom: 1px solid variables.$gray300;
@media (min-width: 768px) {
padding: 2rem 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

padding: 2rem 0 2rem 2rem;
or at least:

padding: 2rem;
padding-right: 0;

<ng-container *ngIf="userGists$ | async as gists">
<ng-container *ngIf="gists.length > 0">
<div class="list-container">
<ng-container *ngIf="gists.length > 0; else noGists">
Copy link
Contributor

Choose a reason for hiding this comment

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

gists?.length
gists can be undefined or null here

Copy link
Contributor

Choose a reason for hiding this comment

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

i would propose ngIf="(gists?.length)" . it is shorter and unbreakable :)

aside {
background: variables.$white;
padding: 2rem;
@media (max-width: 768px) {
Copy link
Contributor

Choose a reason for hiding this comment

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

put the breakpoints to variables.scss and reuse them

Copy link
Contributor

Choose a reason for hiding this comment

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

Or at least to local variables if they are not needed all over the app.

gist.fileName
}}</a>
<li>
<a class="link" [href]="gist.url" target="_blank">{{
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: move {{ gist.fileName }} to separate line for readability. + it also can conflict with linting rules

@SDaian SDaian requested a review from ihardz July 12, 2022 14:47
}}</a>
</ng-container>
</div>
<ng-container *ngIf="(gists?.length); else noGists">
Copy link
Contributor

@ihardz ihardz Jul 12, 2022

Choose a reason for hiding this comment

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

remove excess container. Place the condition right on ui

<li *ngFor="let gist of gists">
<a class="link" [href]="gist.url" target="_blank">
{{ gist.fileName }}</a
>
Copy link
Contributor

@ihardz ihardz Jul 12, 2022

Choose a reason for hiding this comment

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

format the braces (think about linter rules)

<a class="link" [href]="gist.url" target="_blank">
  {{ gist.fileName }}
</a>

@SDaian SDaian requested a review from ihardz July 12, 2022 15:04
@SDaian SDaian merged commit c652e9b into main Jul 12, 2022
@SDaian SDaian deleted the angular-ngrx-scss/gists-panel-improvements branch July 12, 2022 15:07
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.

[Angular - NgRx - SCSS] Create gists panel

2 participants