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

Fix validation errors on lists page #4482

Merged
merged 7 commits into from
Jan 29, 2021
Merged

Fix validation errors on lists page #4482

merged 7 commits into from
Jan 29, 2021

Conversation

SaravgiYash
Copy link
Contributor

Closes #4472

Stakeholders

@jdlrobson

@jdlrobson jdlrobson self-requested a review January 24, 2021 22:22
@jdlrobson jdlrobson self-assigned this Jan 24, 2021
jdlrobson
jdlrobson requested changes Jan 24, 2021
View reviewed changes
Copy link
Collaborator

@jdlrobson jdlrobson left a comment

Choose a reason for hiding this comment

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

The ID on the body tag is also problematic IMO, and we should seek to remove it as part of these changes.

bundlesize.config.json Outdated Show resolved Hide resolved
openlibrary/templates/lists/home.html Outdated Show resolved Hide resolved
@SaravgiYash
Copy link
Contributor Author

@jdlrobson, I have made the required changes.

jdlrobson
jdlrobson requested changes Jan 25, 2021
View reviewed changes
Copy link
Collaborator

@jdlrobson jdlrobson left a comment

Choose a reason for hiding this comment

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

Please update body# selectors in CSS .

@SaravgiYash
Copy link
Contributor Author

@jdlrobson, some styles are directly applied to the body# selectors, so how should I go about it?
image

@jdlrobson
Copy link
Collaborator

@jdlrobson, some styles are directly applied to the body# selectors, so how should I go about it?

Note the body id selectors predate the page specific CSS. page-form is currently only ever applying to pages with the ID form on the body, so this means you can safely do

body {
 text-align: left;
 ...
}

For the nested CSS, it should be fine to move them out of the body tag and remove a level of indentation.

@SaravgiYash
Copy link
Contributor Author

@jdlrobson I have moved the internal CSS. The color mentioned are not present in colors.less so should I add them?

I was wondering how all these CSS are linked to the template.html files?

@jdlrobson
Copy link
Collaborator

@jdlrobson I have moved the internal CSS. The color mentioned are not present in colors.less so should I add them?

I was wondering how all these CSS are linked to the template.html files?

The magic happens in openlibrary/templates/site/head.html
The bodyid is used to generate the name of the stylesheet to be added to the page.

Ideally we'd have less of these page specific stylesheets but there's a lot of clean up to do first.

jdlrobson
jdlrobson previously requested changes Jan 27, 2021
View reviewed changes
Copy link
Collaborator

@jdlrobson jdlrobson left a comment

Choose a reason for hiding this comment

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

There are some linting issues. Run npm run lint locally.

Will pull this down for testing once that's done.

@SaravgiYash
Copy link
Contributor Author

@jdlrobson what about color-no-hex rule?

@jdlrobson
Copy link
Collaborator

@jdlrobson what about color-no-hex rule?

Make sure you use one of the variables in static/css/less/colors.less. Add a new one if necessary.

@SaravgiYash
Copy link
Contributor Author

@jdlrobson Now you can review this PR

@jdlrobson jdlrobson self-requested a review January 29, 2021 02:05
@jdlrobson jdlrobson dismissed their stale review January 29, 2021 02:10

Looking at Css changes now

jdlrobson
jdlrobson requested changes Jan 29, 2021
View reviewed changes
static/css/legacy-borrowTable-adminUser.less Outdated Show resolved Hide resolved
openlibrary/templates/borrow_admin.html Show resolved Hide resolved
static/css/legacy-borrowTable-adminUser.less Outdated Show resolved Hide resolved
jdlrobson
jdlrobson reviewed Jan 29, 2021
View reviewed changes
static/css/page-plain.less
}
// Specific to borrowTable component
/* stylelint-disable selector-max-specificity */
#borrowTable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps in a follow up we could move this into legacy-borrowTable-plain.less for consistency with legacy-borrowTable-adminUser.less.

The eventual goal would be to unify these styles with those in legacy-borrowTable-adminUser.less and static/css/components/borrowTable.less

@jdlrobson jdlrobson merged commit f996715 into internetarchive:master Jan 29, 2021
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 5, 2021
@SaravgiYash @Sabreen-Parveen
Fix validation errors on lists page (internetarchive#4482)
97ee754 
* Removed Inlined CSS from borrow_admin.html and moved into stylesheets
* Removed body id from all pages
* Updated body# selectors with styles in associated page entry points

Fixes: internetarchive#4482
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 5, 2021
@SaravgiYash @Sabreen-Parveen
Fix validation errors on lists page (internetarchive#4482)
fcce560 
* Removed Inlined CSS from borrow_admin.html and moved into stylesheets
* Removed body id from all pages
* Updated body# selectors with styles in associated page entry points

Fixes: internetarchive#4482
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 5, 2021
@SaravgiYash @Sabreen-Parveen
Fix validation errors on lists page (internetarchive#4482)
03c5227 
* Removed Inlined CSS from borrow_admin.html and moved into stylesheets
* Removed body id from all pages
* Updated body# selectors with styles in associated page entry points

Fixes: internetarchive#4482
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 5, 2021
@SaravgiYash @Sabreen-Parveen
Fix validation errors on lists page (internetarchive#4482)
032e2f3 
* Removed Inlined CSS from borrow_admin.html and moved into stylesheets
* Removed body id from all pages
* Updated body# selectors with styles in associated page entry points

Fixes: internetarchive#4482
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 5, 2021
@SaravgiYash @Sabreen-Parveen
Fix validation errors on lists page (internetarchive#4482)
2822d84 
* Removed Inlined CSS from borrow_admin.html and moved into stylesheets
* Removed body id from all pages
* Updated body# selectors with styles in associated page entry points

Fixes: internetarchive#4482
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 15, 2021
@SaravgiYash @Sabreen-Parveen
Fix validation errors on lists page (internetarchive#4482)
12ecff1 
* Removed Inlined CSS from borrow_admin.html and moved into stylesheets
* Removed body id from all pages
* Updated body# selectors with styles in associated page entry points

Fixes: internetarchive#4482
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdlrobson jdlrobson jdlrobson approved these changes

Assignees

@jdlrobson jdlrobson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Lists page has duplicate lists id
2 participants
@SaravgiYash @jdlrobson