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.

Sign up for GitHub

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

Jump to bottom

Introduce experimental grid support #567

Merged
merged 2 commits into from
Feb 15, 2018
Merged

Introduce experimental grid support #567

merged 2 commits into from
Feb 15, 2018

Conversation

joshblack
Copy link
Contributor

@joshblack joshblack commented Feb 14, 2018 •
edited
Loading

Overview

Adds support for the experimental grid into carbon.

Added

  • src/globals/grid/experimental directory added with experimental grid specs

Changed

  • src/globals/grid/_grid.scss moved to src/globals/grid/classic
  • src/global/grid/_grid.scss exports the grid module and toggles imports for experimental

Removed

Testing / Reviewing

  • Verify that by default the CSS outputs doesn't include experimental code
  • Verify that setting css--use-experimental-grid to true includes the default experimental grid code
  • Verify that setting css--use-experimental-grid and css--use-experimental-grid-fallback both to true includes the fallback for the experimental grid in addition to the default implementation

@joshblack
Copy link
Contributor Author

cc @seejamescode

@seejamescode
Copy link
Contributor

Looks good to me! I am guessing you intend for future flags to be css--use-experimental-{element}.

Is Carbon’s rem also 16px?

The only thing that seems important to me is a devDependency for css-gridish. Then we can easily receive updates. I am respecting semver with it’s versioning.

asudoh
asudoh approved these changes Feb 14, 2018
View reviewed changes
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Look perfect - Thanks @joshblack! 👍

@diego-codes
Copy link
Contributor

This looks good to me!

alisonjoseph
alisonjoseph approved these changes Feb 15, 2018
View reviewed changes
Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

LGTM too! 👍

tw15egan
tw15egan approved these changes Feb 15, 2018
View reviewed changes
Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

🙌

@tw15egan tw15egan merged commit 91db595 into carbon-design-system:master Feb 15, 2018
@joshblack joshblack deleted the feat/add-experimental-grid branch February 15, 2018 20:15
@seejamescode
Copy link
Contributor

Hey y’all, do you sometimes do multiple PRs for a single issue? We had a few more requirements in the issue.

Also, is Carbon’s rem size also 16px? if not, this may break some layouts.

@joshblack
Copy link
Contributor Author

Hey @seejamescode! I think this was just merged in early, no sweat. We definitely do multiple PRs for one issue though, with the goal of trying to keep PRs smaller.

Looking at css-gridish stuff now, is there a way to specify what gets generated? For example, we wouldn't want to output a sketch file in the build step for the grid files.

@seejamescode
Copy link
Contributor

No, we should make that a config option over at css-gridish. In the meantime, I would delete those files in the script.

@tw15egan
Copy link
Collaborator

I'd prefer to merge that in once we can control what gets generated. And yes, our rem size is 16px

@joshblack
Copy link
Contributor Author

Made an issue on css-gridish to discuss what could help out here: IBM/css-gridish#20 😄

joshblack pushed a commit to joshblack/carbon that referenced this pull request May 2, 2019
@asudoh @tw15egan
refactor(FloatingMenu): improved position calculation logic (carbon-d…
d5f541b 
…esign-system#567)

Did the following:

* Made some changes to calculate the position in the right phase in the component lifecycle
* Removed several hard-coded geometries to accomodate future style changes
* Hoisted several logic as non-class functions so that they are independent of class state
* Enhanced `menuOffset` prop in `<FloatingMenu>` to support a callback to return the offset,
  e.g. to give `<OverflowMenu>` a chance to calculate the adjustment of non-centered menu arrow
* Enhanced `<Icon>` so that `<svg>` in it can be grabbed, to allow calculating tooltip position based on that,
  and the padding in the outer container doesnot affect the positioning

These would make us easier to make future enhancement to `<FloatingMenu>`,
easing the complexity introduced to support code paths of with/without React portal API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tw15egan tw15egan tw15egan approved these changes

@alisonjoseph alisonjoseph alisonjoseph approved these changes

@asudoh asudoh asudoh approved these changes

@marijohannessen marijohannessen Awaiting requested review from marijohannessen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@joshblack @seejamescode @diego-codes @tw15egan @asudoh @alisonjoseph