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

feat(stacked-bar-chart): Create component #988

Merged
merged 1 commit into from Jun 29, 2020

Conversation

subarroca
Copy link
Contributor

@subarroca subarroca commented May 12, 2020

Pull Request

Stacked bar chart without value axis. It should be addressed in the future
E2E are to come this week or next one

Type of PR

Feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING doc and I follow the PR guidelines
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@subarroca
Copy link
Contributor Author

Closes #884

@subarroca subarroca force-pushed the feature/stacked-bar-chart branch 2 times, most recently from f0ad8aa to f217dd9 Compare May 18, 2020 09:22
@tomheller tomheller linked an issue May 20, 2020 that may be closed by this pull request
Copy link
Collaborator

@tomheller tomheller left a comment

Choose a reason for hiding this comment

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

Additional notes

Overlay does not seem to work in 'mode: column'. I've tested this in the dev example you provided and it does not show up for me.

Hover styles and cursor: pointer on the legend to indicate that it can be clicked and disabled could be added.

If an element is selected, and the data is hidden by clicking on the label, the selection stays there. Should the selection clear in this case?

Selection breaks if you follow these steps to reproduce in the given example:

  1. Load the example in column mode
  2. Select any of the other elements in the chart
  3. Switch mutilple to false
  4. Switch mode to column
    You can no longer change the selection...

@subarroca
Copy link
Contributor Author

subarroca commented May 21, 2020

@tomheller About the additional notes

  • Overlays simplified in upcoming commit

  • If an element is selected, and the data is hidden by clicking on the label, the selection stays there. Should the selection clear in this case? I think it should, as you don't remove the selection, you only change the view of the chart

  • The failing example you provided is not reproducible for me. Could you try it again once the changes are available?

Also:

  • aria Label and E2E to come next week

@tomheller
Copy link
Collaborator

* Overlays simplified in upcoming commit

Nice, I'm looking forward to this.

* **If an element is selected, and the data is hidden by clicking on the label, the selection stays there. Should the selection clear in this case?** I think it should, as you don't remove the selection, you only change the view of the chart

That works for me, I just wanted to check.

* The failing example you provided is not reproducible for me. Could you try it again once the changes are available?

Yes, it is still reproduceable. I cannot select any other series when changed to column mode.

* aria Label and E2E to come next week

Very nice, 👍


Please just re-request a review if you want me to look over the pr again.
image

@subarroca
Copy link
Contributor Author

Ok, I think it's complete now.
Added:

  • E2E
  • Aria label
  • Value axis with absolute and relative values

@subarroca subarroca requested a review from tomheller May 29, 2020 14:10
tomheller
tomheller previously approved these changes Jun 2, 2020
libs/barista-components/stacked-series-chart/README.md Outdated Show resolved Hide resolved
libs/barista-components/stacked-series-chart/README.md Outdated Show resolved Hide resolved
libs/barista-components/stacked-series-chart/README.md Outdated Show resolved Hide resolved
@tomheller tomheller force-pushed the feature/stacked-bar-chart branch 2 times, most recently from c14e1b1 to 5882a5d Compare June 9, 2020 12:12
Copy link
Collaborator

@tomheller tomheller left a comment

Choose a reason for hiding this comment

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

We have tried to get the checks running in the CI. It seems that the unit test on the ci is way faster, and we may run into some timing issues, that you would not run into on your local machine.

This hints at some implementation hiccups.
This PR will have to wait until we have proper time to figure out what is wrong here. All in all it is a weird behaviour.

@tomheller
Copy link
Collaborator

Hi @subarroca!
I dug deep into the ci yesterday and was not quite successful. Although we might have narrowed it down to something weird within the angular lifecycle and timing problems on the CI.
I want to check into this a little bit more, but it would require me to adapt a bit of your overlay code logic. I will do this probably on monday next week (as I'm quite busy for the rest of this week) and I would add a the changes in a separate commit, if this is fine for you?

@subarroca
Copy link
Contributor Author

subarroca commented Jun 10, 2020 via email

@tomheller
Copy link
Collaborator

If this is not the right way to implement the overlay we should change it
too in the radial chart as I took the implementation from there

Good point, I will have a look at this as well .

@ffriedl89
Copy link
Collaborator

ffriedl89 commented Jun 10, 2020

That these tests are failing only on the CI docker images is really a mystery to me. Thanks to @tomheller for to making an effort to figure this out! And thanks @subarroca for the patience!

tomheller
tomheller previously approved these changes Jun 16, 2020
@lukasholzer lukasholzer mentioned this pull request Jun 19, 2020
4 tasks
Copy link
Contributor

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

Thank you very much for contributing a such high quality component!
Verry well documented! :)

Please add a package.json inside the folder with this content:

{
  "ngPackage": {
    "lib": {
      "entryFile": "index.ts"
    }
  }
}

Otherwise it won't be packaged by the ngPackagr and cannot not be released.

To check if your component is build run npm run ng build components then go to the dist folder and check out the generated bundle.

For your next contribution maybe read about the symbold ordering https://github.com/dynatrace-oss/barista/blob/master/CODING_STANDARDS.md#symbol-ordering-in-libsbarista-componentsdirectivesservices in our coding standards.

This would make reviewing a little bit easier if you group your methods and properties in the components :) and would align it with the other components.

@lukasholzer lukasholzer added the pr: needs-changes This PR has been reviewed and needs changes before it can be merged label Jun 19, 2020
@subarroca
Copy link
Contributor Author

Thanks for the reviews
@lukasholzer, I reordered the component members following the guide but no functionality should be affected

@tomheller
Copy link
Collaborator

@subarroca
It seems you still have a build error. One of the functions called from the template has a parameter mismatch

------------------------------------------------------------------------------
 
Building entry point '@dynatrace/barista-components/stacked-series-chart'
 
------------------------------------------------------------------------------
 
Compiling TypeScript sources through ngc
 
WARNING: autoprefixer: /home/circleci/barista/libs/barista-components/stacked-series-chart/src/stacked-series-chart.scss:10:3: IE does not support align-items on grid containers. Try using align-self on child elements instead: :host > * { align-self: center }
 
WARNING: autoprefixer: /home/circleci/barista/libs/barista-components/stacked-series-chart/src/stacked-series-chart.scss:20:3: grid-auto-flow: dense is not supported by IE
 
WARNING: autoprefixer: /home/circleci/barista/libs/barista-components/stacked-series-chart/src/stacked-series-chart-column.scss:11:3: IE does not support justify-items on grid containers. Try using justify-self on child elements instead: :host(.dt-stacked-series-chart-column) .dt-stacked-series-chart-container > * { justify-self: center }
 
WARNING: autoprefixer: /home/circleci/barista/libs/barista-components/stacked-series-chart/src/stacked-series-chart-column.scss:52:3: grid-auto-rows is not supported by IE
 
WARNING: autoprefixer: /home/circleci/barista/libs/barista-components/stacked-series-chart/src/stacked-series-chart-bar.scss:11:3: IE does not support align-items on grid containers. Try using align-self on child elements instead: :host(.dt-stacked-series-chart-bar) .dt-stacked-series-chart-container > * { align-self: center }
 
ERROR: libs/barista-components/stacked-series-chart/src/stacked-series-chart.html(54,9): Expected 0 arguments, but got 1.

@tomheller tomheller removed the pr: needs-changes This PR has been reviewed and needs changes before it can be merged label Jun 29, 2020
@tomheller tomheller added the pr: needs-cherry-pick When a pull request needs manual cherry picking label Jun 29, 2020
@ffriedl89 ffriedl89 merged commit fded8eb into dynatrace-oss:master Jun 29, 2020
@subarroca subarroca deleted the feature/stacked-bar-chart branch June 29, 2020 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: needs-cherry-pick When a pull request needs manual cherry picking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Single stacked bar chart
4 participants