Details

Importing chosen-js in swh/web/assets/src/bundles/vendors/index.js doesn't import the css. On adding imports for css and png separately, webpack fails. I am not able to figure out why ?

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/207/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/207/console

Harbormaster failed remote builds in B6880: Diff 5841!Jul 15 2019, 9:34 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/208/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/208/console

Fix webpack error and trigger updateLanguage with dropdown

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/614/ for more details.

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/209/ for more details.

Harbormaster completed remote builds in B6886: Diff 5850.Jul 16 2019, 1:12 PM

CSS needs to be updated for the dropdown.
Rest everything is done.

swh/web/assets/config/webpack.config.development.js
297 ↗	(On Diff #5850)	Not sure why (\?v=\d+\.\d+\.\d+) is used here. Should this be used with images also?
311 ↗	(On Diff #5850)	Will directory 'img/imports' be fine for outputPath?

vlorentz added a subscriber: vlorentz.Jul 16 2019, 2:36 PM

vlorentz added inline comments.

swh/web/assets/config/webpack.config.development.js
297 ↗	(On Diff #5850)	I don't know... don't use it if it's not needed.
306 ↗	(On Diff #5850)	Do we need GIF and JPEG for now?
311 ↗	(On Diff #5850)	What about `img/thirdParty` (to match the name used elsewhere)?
swh/web/templates/includes/top-navigation.html
134 ↗	(On Diff #5850)	What does this line do?

kalpitk added inline comments.Jul 16 2019, 2:45 PM

swh/web/assets/config/webpack.config.development.js
297 ↗	(On Diff #5850)	ok
306 ↗	(On Diff #5850)	No. Should I remove gif and jpeg from here?
311 ↗	(On Diff #5850)	ok
swh/web/templates/includes/top-navigation.html
134 ↗	(On Diff #5850)	It was for initialising chosen on select. It works without this line, because of the next line. Will remove it.

vlorentz added inline comments.Jul 16 2019, 3:41 PM

swh/web/assets/config/webpack.config.development.js
306 ↗	(On Diff #5850)	If we don't need it, yes.

Add css

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/615/ for more details.

I had to use !important everywhere to override the default css. I guess, there must be a way to avoid it.

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/210/ for more details.

Harbormaster completed remote builds in B6890: Diff 5854.Jul 16 2019, 7:44 PM

kalpitk retitled this revision from [WIP] Add option to select code language to Add option to select code language.Jul 17 2019, 1:22 PM

Update CSS and refactor

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/616/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/616/console

Harbormaster failed remote builds in B6893: Diff 5857!Jul 17 2019, 1:27 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/211/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/211/console

Fix failing tests

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/617/ for more details.

Harbormaster completed remote builds in B6895: Diff 5859.Jul 17 2019, 3:02 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/212/ for more details.

Update commit msg

kalpitk retitled this revision from [WIP] Add option to select code language to Add option to select code language.Jul 17 2019, 3:04 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/618/ for more details.

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/213/ for more details.

Harbormaster completed remote builds in B6896: Diff 5860.Jul 17 2019, 3:10 PM

It's really nice!

Just a few remarks:

it's unclear what language_select does, especially as it's used both as a list of languages and a boolean to tell whether the language selector should be shown. It should be renamed to available_languages

swh/web/browse/views/utils/snapshot_context.py
457 ↗	(On Diff #5860)	"Override language with user-selected language"
swh/web/templates/includes/content-display.html
68	This currently works because there are no other params, but it would be nice to support other parameters in the URL, so we don't get unexpected bugs later.
swh/web/templates/includes/top-navigation.html
134 ↗	(On Diff #5860)	`{{ language }}` seems to be sanitized (when I try values with quotes, eg. http://127.0.0.1:5004/browse/origin/https://github.com/wcoder/highlightjs-line-numbers.js/content/gulpfile.js/?language=foo%22);%20alert(%22hello ), but where?

kalpitk added inline comments.Jul 17 2019, 8:19 PM

swh/web/browse/views/utils/snapshot_context.py
457 ↗	(On Diff #5860)	ok
swh/web/templates/includes/content-display.html
68	I tested it with other params like origin_url, it works. I mean for links like "http://127.0.0.1:5004/browse/content/sha1_git:33217f73631dbfa1860b41060e5a6a5a62420f69/?origin_url=https%3A%2F%2Fgithub.com%2Fusartkom%2Flearning_pyhon&language=gherkin", everything works. (URLSearchParams stores all params in url)
swh/web/templates/includes/top-navigation.html
134 ↗	(On Diff #5860)	Django auto-escapes template variables by default.