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

670 - Toolbar children: set horizontal and vertical layout #699

Merged
merged 6 commits into from
Apr 10, 2019

Conversation

msssk
Copy link
Contributor

@msssk msssk commented Mar 23, 2019

Type: feature

The following has been addressed in the PR:

  • There is a related issue
  • All code matches the style guide
  • Unit tests are updated in the PR

Description:
Toolbar: add CSS classes and rules to lay out children using flex-direction: row for the toolbar and flex-direction: column for the slide pane.

Resolves #670

@@ -81,6 +81,10 @@ export class Toolbar extends I18nMixin(ThemedMixin(WidgetBase))<ToolbarPropertie
classes,
heading
} = this.properties;
const actions = v('div', {
classes: this.theme(this._collapsed ? css.slidePaneActions : css.actions),
Copy link
Member

Choose a reason for hiding this comment

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

I think that we can always use css.actions here. We can differentiate between slide pane / toolbar placement via the collapsed class. This makes the dom / classes more predictable for the end user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this is definitely a better solution! Updated

@codecov
Copy link

codecov bot commented Mar 29, 2019

Codecov Report

Merging #699 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #699      +/-   ##
==========================================
- Coverage   99.04%   99.04%   -0.01%     
==========================================
  Files          45       45              
  Lines        3342     3334       -8     
  Branches      926      925       -1     
==========================================
- Hits         3310     3302       -8     
  Misses         32       32
Impacted Files Coverage Δ
src/toolbar/index.ts 95% <100%> (+0.08%) ⬆️
src/select/index.ts 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b8cc08...bf161c0. Read the comment docs.

@msssk msssk force-pushed the 670-toolbar-layout branch from 6656075 to 5273813 Compare April 9, 2019 17:35
}

.collapsed {
& .actions {
Copy link
Member

@tomdye tomdye Apr 9, 2019

Choose a reason for hiding this comment

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

sorry to be a pain, but we're avoiding the use of nested selectors via the & operator. Could you change this to .collapsed .actions please then we can get this merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in bf161c0

@tomdye tomdye merged commit 25af83f into dojo:master Apr 10, 2019
@tomdye
Copy link
Member

tomdye commented Apr 10, 2019

Thanks @msssk

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.

2 participants