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

[MAJOR] use attribute hidden #691

Merged
merged 2 commits into from
Oct 24, 2019
Merged

[MAJOR] use attribute hidden #691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
054d723
use attribute hidden
tinovyatkin Oct 24, 2019
a48cf0e
commit something to re-run tests
tinovyatkin Oct 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
# Contributions
# Contributions
In lieu of a formal styleguide, take care to maintain the existing coding style ensuring there are no linting errors. Add unit tests for any new or changed functionality. Lint and test your code using the npm scripts below:

### NPM tasks
| Task | Usage |
| -------------------- | ------------------------------------------------------------ |
| `npm run start` | Fire up local server for development |
| `npm run test` | Run sequence of tests once |
| `npm run test:unit` | Run sequence of unit tests once |
| `npm run test:e2e` | Run sequence of integration tests once |
| `npm run test:watch` | Fire up test server and re-test on file change |
| `npm run js:build` | Compile Choices to an uglified JavaScript file |
| `npm run css:watch` | Watch SCSS files for changes. On a change, run build process |
Expand Down
2 changes: 0 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,6 @@ will be returned. If you target one element, that instance will be returned.
openState: 'is-open',
disabledState: 'is-disabled',
highlightedState: 'is-highlighted',
hiddenState: 'is-hidden',
flippedState: 'is-flipped',
loadingState: 'is-loading',
noResults: 'has-no-results',
Expand Down Expand Up @@ -518,7 +517,6 @@ classNames: {
openState: 'is-open',
disabledState: 'is-disabled',
highlightedState: 'is-highlighted',
hiddenState: 'is-hidden',
flippedState: 'is-flipped',
selectedState: 'is-highlighted',
}
Expand Down
6 changes: 3 additions & 3 deletions cypress/integration/select-multiple.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ describe('Choices - select multiple', () => {

it('updates the value of the original input', () => {
cy.get('[data-test-hook=basic]')
.find('.choices__input.is-hidden')
.find('.choices__input[hidden]')
.should($select => {
expect($select.val()).to.contain(selectedChoiceText);
});
Expand Down Expand Up @@ -150,7 +150,7 @@ describe('Choices - select multiple', () => {

it('updates the value of the original input', () => {
cy.get('[data-test-hook=basic]')
.find('.choices__input.is-hidden')
.find('.choices__input[hidden]')
.should($select => {
const val = $select.val() || [];
expect(val).to.not.contain(removedChoiceText);
Expand Down Expand Up @@ -806,7 +806,7 @@ describe('Choices - select multiple', () => {

it('updates the value of the original input', () => {
cy.get('[data-test-hook=set-choice-by-value]')
.find('.choices__input.is-hidden')
.find('.choices__input[hidden]')
.should($select => {
const val = $select.val() || [];
expect(val).to.contain(dynamicallySelectedChoiceValue);
Expand Down
4 changes: 2 additions & 2 deletions cypress/integration/select-one.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ describe('Choices - select one', () => {

it('updates the value of the original input', () => {
cy.get('[data-test-hook=remove-button]')
.find('.choices__input.is-hidden')
.find('.choices__input[hidden]')
.should($select => {
const val = $select.val() || [];

Expand Down Expand Up @@ -818,7 +818,7 @@ describe('Choices - select one', () => {

it('updates the value of the original input', () => {
cy.get('[data-test-hook=set-choice-by-value]')
.find('.choices__input.is-hidden')
.find('.choices__input[hidden]')
.should($select => {
const val = $select.val() || [];
expect(val).to.contain(dynamicallySelectedChoiceValue);
Expand Down
4 changes: 2 additions & 2 deletions cypress/integration/text.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe('Choices - text element', () => {
.type('{enter}');

cy.get('[data-test-hook=basic]')
.find('.choices__input.is-hidden')
.find('.choices__input[hidden]')
.should('have.value', textInput);
});

Expand Down Expand Up @@ -151,7 +151,7 @@ describe('Choices - text element', () => {
.click();

cy.get('[data-test-hook=remove-button]')
.find('.choices__input.is-hidden')
.find('.choices__input[hidden]')
.then($input => {
expect($input.val()).to.not.contain(textInput);
});
Expand Down
6 changes: 2 additions & 4 deletions src/scripts/components/wrapped-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export default class WrappedElement {
conceal() {
// Hide passed input
this.element.classList.add(this.classNames.input);
this.element.classList.add(this.classNames.hiddenState);
this.element.hidden = true;

// Remove element from tab index
this.element.tabIndex = '-1';
Expand All @@ -35,14 +35,13 @@ export default class WrappedElement {
this.element.setAttribute('data-choice-orig-style', origStyle);
}

this.element.setAttribute('aria-hidden', 'true');
this.element.setAttribute('data-choice', 'active');
}

reveal() {
// Reinstate passed element
this.element.classList.remove(this.classNames.input);
this.element.classList.remove(this.classNames.hiddenState);
this.element.hidden = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it not be better to remove the attribute here?

Copy link
Contributor Author

@tinovyatkin tinovyatkin Oct 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It's generally bad practice to mix simplified accessors (like .hidden or .tabIndex) with getAttribute/setAttribute. It does the same job, but confuses developers. For example in this case this.element.hidden = false will actually remove the attribute from DOM element, because hidden is a boolean attribute (so, there is no such thing as hidden="false" attribute)

PS: You may push something into this PR, so, GitHub Action will re-run again with Codecov coverage report (it doesn't give access to secrets for forked repositories, as mine)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah didn't realise that will remove it - fair enough 👍

this.element.removeAttribute('tabindex');

// Recover original styles if any
Expand All @@ -54,7 +53,6 @@ export default class WrappedElement {
} else {
this.element.removeAttribute('style');
}
this.element.removeAttribute('aria-hidden');
this.element.removeAttribute('data-choice');

// Re-assign values - this is weird, I know
Expand Down
9 changes: 2 additions & 7 deletions src/scripts/components/wrapped-element.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,7 @@ describe('components/wrappedElement', () => {
expect(
instance.element.classList.contains(instance.classNames.input),
).to.equal(true);
expect(
instance.element.classList.contains(instance.classNames.hiddenState),
).to.equal(true);
expect(instance.element.getAttribute('aria-hidden')).to.equal('true');
expect(instance.element.hidden).to.be.true;
expect(instance.element.getAttribute('data-choice')).to.equal('active');
expect(instance.element.getAttribute('data-choice-orig-style')).to.equal(
originalStyling,
Expand All @@ -78,9 +75,7 @@ describe('components/wrappedElement', () => {
expect(
instance.element.classList.contains(instance.classNames.input),
).to.equal(false);
expect(
instance.element.classList.contains(instance.classNames.hiddenState),
).to.equal(false);
expect(instance.element.hidden).to.be.false;
expect(instance.element.getAttribute('style')).to.equal(originalStyling);
expect(instance.element.getAttribute('aria-hidden')).to.equal(null);
expect(instance.element.getAttribute('data-choice')).to.equal(null);
Expand Down
1 change: 0 additions & 1 deletion src/scripts/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ export const DEFAULT_CLASSNAMES = {
openState: 'is-open',
disabledState: 'is-disabled',
highlightedState: 'is-highlighted',
hiddenState: 'is-hidden',
flippedState: 'is-flipped',
loadingState: 'is-loading',
noResults: 'has-no-results',
Expand Down
1 change: 0 additions & 1 deletion src/scripts/constants.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ describe('constants', () => {
'openState',
'disabledState',
'highlightedState',
'hiddenState',
'flippedState',
'loadingState',
'noResults',
Expand Down
36 changes: 16 additions & 20 deletions src/styles/base.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,23 @@
= Generic styling =
=============================================*/

$global-guttering : 24px;
$global-font-size-h1 : 32px;
$global-font-size-h2 : 24px;
$global-font-size-h3 : 20px;
$global-font-size-h4 : 18px;
$global-font-size-h5 : 16px;
$global-font-size-h6 : 14px;
$global-guttering: 24px;
$global-font-size-h1: 32px;
$global-font-size-h2: 24px;
$global-font-size-h3: 20px;
$global-font-size-h4: 18px;
$global-font-size-h5: 16px;
$global-font-size-h6: 14px;

* {
-webkit-font-smoothing: antialiased;
-moz-osx-font-smoothing: grayscale
-moz-osx-font-smoothing: grayscale;
}

*,
*:before,
*:after {
box-sizing: border-box
box-sizing: border-box;
}

html,
Expand All @@ -30,10 +30,10 @@ body {
}

body {
font-family: "Helvetica Neue", Helvetica, Arial, "Lucida Grande", sans-serif;
font-family: 'Helvetica Neue', Helvetica, Arial, 'Lucida Grande', sans-serif;
font-size: 16px;
line-height: 1.4;
color: #FFFFFF;
color: #ffffff;
background-color: #333;
overflow-x: hidden;
}
Expand All @@ -52,7 +52,7 @@ p {

hr {
display: block;
margin: $global-guttering*1.25 0;
margin: $global-guttering * 1.25 0;
border: 0;
border-bottom: 1px solid #eaeaea;
height: 1px;
Expand All @@ -73,7 +73,7 @@ h6 {
a,
a:visited,
a:focus {
color: #FFFFFF;
color: #ffffff;
text-decoration: none;
font-weight: 600;
}
Expand Down Expand Up @@ -133,14 +133,14 @@ label + p {
display: block;
margin: auto;
max-width: 40em;
padding: $global-guttering*2;
padding: $global-guttering * 2;
@media (max-width: 620px) {
padding: 0;
}
}

.section {
background-color: #FFFFFF;
background-color: #ffffff;
padding: $global-guttering;
color: #333;
a,
Expand Down Expand Up @@ -184,12 +184,8 @@ label + p {
text-align: center;
}

.is-hidden {
display: none;
}

[data-test-hook] {
margin-bottom: $global-guttering;
}

/*===== End of Section comment block ======*/
/*===== End of Section comment block ======*/
Loading