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

Refactor #275

Merged
merged 316 commits into from
Oct 24, 2019
Merged

Refactor #275

merged 316 commits into from
Oct 24, 2019

Conversation

theiliad
Copy link
Member

@theiliad theiliad commented Jun 10, 2019

This is a WIP for the re-imagination of the charting components. As we make progress here I will reflect the updates in bullet point format.

Updates:

  • New rendering engine
  • Services
  • Model
  • Time-series
  • Dynamic axis & scales
  • Restyle of existing components
  • New color palette
  • Tooltip variations

Closes #305
Closes #191
Closes #165
Closes #173
Closes #181
Closes #186
Closes #238
Closes #310
Closes #304
Closes #279
Closes #278
Closes #277
Closes #258
Closes #113
Closes #327
Closes #360
Closes #377
Closes #378

@theiliad theiliad self-assigned this Jun 10, 2019
@netlify
Copy link

netlify bot commented Jul 24, 2019

Deploy preview for carbon-charts-core ready!

Built with commit bb4cbec

https://deploy-preview-275--carbon-charts-core.netlify.com

@netlify
Copy link

netlify bot commented Jul 24, 2019

Deploy preview for carbon-charts-react ready!

Built with commit bb4cbec

https://deploy-preview-275--carbon-charts-react.netlify.com

@netlify
Copy link

netlify bot commented Jul 24, 2019

Deploy preview for carbon-charts-angular ready!

Built with commit bb4cbec

https://deploy-preview-275--carbon-charts-angular.netlify.com

@netlify
Copy link

netlify bot commented Jul 24, 2019

Deploy preview for carbon-charts-vue ready!

Built with commit bb4cbec

https://deploy-preview-275--carbon-charts-vue.netlify.com

@theiliad theiliad mentioned this pull request Aug 16, 2019
@mjabbink
Copy link

Thanks @Shixie! I’d say these are all critical to make these sing which is the goal.

Co-Authored-By: Zvonimir Fras <zvonimir.fras@gmail.com>
@natashadecoste
Copy link
Contributor

Hi @natashadecoste Title type style change is recent, so please ref this image. Below is a list of updated type specs:

Chart title: Plex sans semibold, 14/18 px, $text-01
Legend name: Plex sans condensed, 12/16 px, $text-02

Axis title: Plex sans regular, 12/16 px, $text-01
Axis ticks: Plex sans condensed, 12/16 px, $text-02
Pie labels: Plex sans condensed, 12/16 px, $text-02

Tooltip: 
Body: Plex sans condensed, 12/16 px, $text-02
Heading (eg. total): Plex sans condensed semibold, 12/16 px, $text-01

the specs above still have 14 as the chart title, im assuming that is a mistake

@shixiedesign
Copy link

shixiedesign commented Oct 24, 2019

Yes I just verified that the latest build has the correct title! 16px = correct. Thanks @natashadecoste 🙏 you are awesome.

So everything in the first comment are addressed. So Mike's hoping we can get the last 3 fix addressed too, and I'm posting a check list here. Hopefully these are not too hard:

  • Need to make sure graph itself sit inside the graphing area with extra 10% graphing area. When axis are bottom and left, extra area should be top and right. When axis are center&middle aligned, extra area should be all around. This is to avoid situ like below, where bar touches top of graphing area.

image

  • Need more 4px padding between Pie's labels so hover state's grown slice doesn't touch label:

image

  • We need axis names on all demos.

image

@theiliad
Copy link
Member Author

theiliad commented Oct 24, 2019

@shixiedesign addressed 2 out of 3 of your issues. Let's push the extra 10% space discussion either to a call/msg or our weekly cadence.

There are more cases to consider there. We should address those scenarios, then the addition will be quick.

With regards to the refactor, we're running behind schedule now, we should merge, and add more updates in newer PRs

UPDATE: added a basic version of the 10% extra spacing, however we should still discuss in detail

@theiliad theiliad merged commit c0d7259 into carbon-design-system:master Oct 24, 2019
@shinytoyrobots
Copy link

This PR should not have been merged - PRs require two developer approvals and one designer approval in addition to the submitter.

Also, this is not a reviewable PR. A PR should not be closing 18 separate issues, it is impossible for there to be any effective review on that basis.

@theiliad
Copy link
Member Author

theiliad commented Oct 28, 2019

This PR should not have been merged - PRs require two developer approvals and one designer approval in addition to the submitter.

Also, this is not a reviewable PR. A PR should not be closing 18 separate issues, it is impossible for there to be any effective review on that basis.

This PR was in active review for 4 weeks.

Changes were requested, applied and approved.

@zvonimirfras and @cal-smith reviewed the code thoroughly, asked for changes, then approved. This was repeated multiple times.

@natashadecoste also reviewed my portion, and I also reviewed her portion of the PR and asked for changes.

On top of that we had @cameroncalder review from a design perspective, topped by @Shixie doing a final design review and approving. Also Mike Abbink, Sadek Bazarra and Michael Gower were shown the demos multiple times.

In terms of the size of the PR, this was a massive rewrite on lots of modules. We needed this work done moving forward considering where our specifications are going.

I’ve tried my best in involving everyone on the work. @alisonjoseph and @joshblack were also aware of the work. Open to suggestions in the future.

@s100
Copy link
Contributor

s100 commented Oct 29, 2019

@shinytoyrobots This PR has been actively worked on throughout June, July, August, September and October, so you may have missed your window for that kind of feedback...

@theiliad Thanks for the major update, I'm looking forward to using the time series line charts!

@theiliad
Copy link
Member Author

@shinytoyrobots This PR has been actively worked on throughout June, July, August, September and October, so you may have missed your window for that kind of feedback...

@theiliad Thanks for the major update, I'm looking forward to using the time series line charts!

Great! Waiting to see how it unfolds in your project 🙂

And again (re. PR), open to any feedback/suggestions in terms of architecture, management etc moving forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants