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

Merge main @ef9d853 #1108

Merged
merged 12 commits into from
Feb 25, 2022
Merged

Merge main @ef9d853 #1108

merged 12 commits into from
Feb 25, 2022

Conversation

julien-deramond
Copy link
Member

@julien-deramond julien-deramond commented Feb 21, 2022

⚠️ The adaptation of CSS variables for buttons in Boosted was complex to do.
🙏 Dear reviewer please check in details:

  • Colors in all states (normal, focused, active, hovered, etc.)
  • Sizes
  • Paddings and margins
  • Everywhere in the doc or components where buttons are used to avoid any regressions!

@julien-deramond julien-deramond added v5 merge Sync with Bootstrap skip:ci labels Feb 21, 2022
@julien-deramond julien-deramond force-pushed the chore/merge-main@cb8726d branch 4 times, most recently from 6d069aa to a613c38 Compare February 24, 2022 09:44
@julien-deramond julien-deramond changed the title Merge main @cb8726d Merge main @ef9d853 Feb 24, 2022
@julien-deramond julien-deramond marked this pull request as ready for review February 24, 2022 09:48
@isabellechanclou
Copy link
Member

isabellechanclou commented Feb 24, 2022

Isabelle's comments:

On docs/5.1/components/buttons:

  • Examples + Dark background + Button tags + Disabled state: Line height is 20px when 18px is indicated in ODS Important button WEB-BTN-IMP-001 => was already like that, but mentioned here for memory => Already indicated in Boosted v4 ⭢ v5 (remaining tasks) #901
  • With icon + Dark background: the label of the button is "Secondary" when it should be "Primary" according to its css class => was already like that, but mentioned here for memory => Already indicated in Boosted v4 ⭢ v5 (remaining tasks) #901
  • Toggle states: there is a possible confusion about the presented components that contain the word 'toggle' in their labels and the ones presented in the 'Toggle buttons with labels' sections in the design system. One may wonder if they are the same components or not. Apparently, the Boosted are inherited from Bootstrap and don't have any equivalent in ODS. If so, it could be useful in Boosted doc to add the warning message saying that these component don't belong to ODS => comment not in the scope of this PR and mentioned here for memory => Creation of Issue Docs > Components > Buttons : Add "not in ODS" alert message for Toggle states #1118

On docs/5.1/components/button-group:

On /docs/5.1/components/pagination/:

  • Working with icons: left and right arrows seem not to be centered in their squares. They seem to be 1 extra pixel towards right for the left arrow and 1 extra pixel towards left for the right arrow. => It is already like that in the current Boosted version + this comment might not be in the scope of this PR for no button css class is used for these components. => Creation of Issue Docs > Components > Pagination > Working with icons: Fix css #1119
  • Disabled and active states: in Boosted, pagination numbers have a margin -left of 10px when they don't in ODS _=> comment not in the scope of this PR and mentioned here for memory => Creation of Issue Docs > Components > Pagination > Disabled and active states: Fix css #1120

LM's one(s):

@julien-deramond
Copy link
Member Author

Small issue here : Close button preview on disabled state (comparison)

Should be fixed with 0ec87a2

@julien-deramond

This comment was marked as resolved.

@julien-deramond
Copy link
Member Author

julien-deramond commented Feb 25, 2022

warning inverse dropdown split buttons are broken

Fixed it with 72f427b. But this modification needs to check non-regression between buttons, dropdowns and split buttons.

Edit: I simplified the box shadows colors with 0abd7db. Otherwise btn-info wouldn't have the good color. This implies that the dark variant dropdown box shadow in now black instead of white; but it seems more coherent with the rest of the components (since we still don't have focus color design specs).

@julien-deramond
Copy link
Member Author

e2ba863 allows to handle more precisely the .btn-links.

Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

LGTM, minor issues can be handled later IMO.

Minor issues detected :

  • .btn-inverse.btn-close:focus is a bit weird : was already here
  • .btn-link outline doesn't move anymore

scss/_button-group.scss Outdated Show resolved Hide resolved
scss/_button-group.scss Show resolved Hide resolved
scss/mixins/_buttons.scss Outdated Show resolved Hide resolved
scss/mixins/_buttons.scss Outdated Show resolved Hide resolved
scss/_buttons.scss Outdated Show resolved Hide resolved
@julien-deramond julien-deramond merged commit 1639d26 into main Feb 25, 2022
@julien-deramond julien-deramond deleted the chore/merge-main@cb8726d branch February 25, 2022 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge Sync with Bootstrap v5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants