Skip to content

Conversation

@jacobneplokh
Copy link
Member

@jacobneplokh jacobneplokh commented Oct 8, 2020

Fixes #22987

Simple changes to the h2 part. Let me know if there is anything wrong, of course :)

Signed-off-by: Jacob Neplokh [email protected]

@rullzer rullzer requested a review from jancborchardt October 8, 2020 07:29
@jancborchardt
Copy link
Member

Hi @jneplokh :) This change would change all h2 styles, and that is a bit much. Could you limit it to only change the h2 of the search emptycontent? As far as I could see, you would simply have to do your changes in this file:
https://github.com/nextcloud/server/blob/master/core/src/views/UnifiedSearch.vue#L745-L748

@jancborchardt
Copy link
Member

@ma12-co did you define the new 15px default as a variable? If not, could you do that, so we can use them elsewhere where we need to bring semantically different elements down visually, like in this case?

@jacobneplokh
Copy link
Member Author

This change would change all h2 styles, and that is a bit much.

Ok, yeah, I was not sure if that was changing too much, but opened a PR anyways (as if I am wrong, I would get corrected, like now 😄)

As far as I could see, you would simply have to do your changes in this file:
https://github.com/nextcloud/server/blob/master/core/src/views/UnifiedSearch.vue#L745-L748

Unfortunately, when I added font-weight: normal; and even font-size: 40px; (the latter is just to further test it) on my dev server, the changes were not reflected (while the h2 changes are). I imagine I have to do something in order to reflect the changes?

Also, I was hoping to close #22978 in the same PR as they seem related. Where would the code be to change the status font size and font weight? Or would it be better to open a separate PR?

Thank you!

@jacobneplokh
Copy link
Member Author

Hey, just bumping this! Do you know what I would have to do to reflect the changes on my dev server?

@jancborchardt
Copy link
Member

@jneplokh sorry for the late reply! If you do changes to parts like search, you need to compile them with this terminal command: npm ci && MODULE=core npm run build :)

@jacobneplokh jacobneplokh changed the title Remove bold font-weight and lower font-size for user dashboard and empty search box Remove bold font-weight and lower font-size for empty search box Oct 27, 2020
@jacobneplokh jacobneplokh force-pushed the fix-bold-and-font-size branch from 727f406 to 9b4d936 Compare October 27, 2020 17:35
@jacobneplokh
Copy link
Member Author

npm ci && MODULE=core npm run build

Awesome, that worked! Just pushed the commit to fix the issue :)

@jacobneplokh jacobneplokh force-pushed the fix-bold-and-font-size branch from 9b4d936 to 493539f Compare October 27, 2020 17:39
@skjnldsv skjnldsv added 2. developing Work in progress design Design, UI, UX, etc. enhancement labels Oct 31, 2020
@skjnldsv
Copy link
Member

Hey! What is the status here?

@jacobneplokh
Copy link
Member Author

Hey! What is the status here?

Hello! It seems to be working (on my end), but there was a failed check: https://github.com/nextcloud/server/pull/23267/checks?check_run_id=1316328378

Not sure what is going on there? Do I have to change something here: https://github.com/nextcloud/server/blob/master/core/js/dist/unified-search.js?

@skjnldsv
Copy link
Member

Yes, you need to ship the compiled files :)

You can rebase and then use the command /compile /core/js/dist as a github.amrom.workers.devment to have a bot do it for you :)

@jacobneplokh jacobneplokh force-pushed the fix-bold-and-font-size branch from 493539f to b04368c Compare October 31, 2020 18:29
@jacobneplokh
Copy link
Member Author

/compile /core/js/dist

@jacobneplokh
Copy link
Member Author

Yes, you need to ship the compiled files :)

You can rebase and then use the command /compile /core/js/dist as a github.amrom.workers.devment to have a bot do it for you :)

Ah ok, got it. Just rebased and commented :)

@jacobneplokh
Copy link
Member Author

Hmm now it seems the static code analysis has failed: https://github.com/nextcloud/server/pull/23267/checks?check_run_id=1336555393

with a few different errors it seems.

@skjnldsv
Copy link
Member

Hmm now it seems the static code analysis has failed: #23267 (checks)

with a few different errors it seems.

Yes, it fails on master, let's ignore :)

@skjnldsv
Copy link
Member

/compile /core/js/dist

Ah, you don't need to use the bot if you already shipped the up-to-date compiled files :)
It seems to pass now! 🚀

@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 31, 2020
@skjnldsv
Copy link
Member

Also @jneplokh, as a good habit, when you create PRs that touches design, you can add screenshots so it's easier to review :)

@jacobneplokh
Copy link
Member Author

jacobneplokh commented Oct 31, 2020

Also @jneplokh, as a good habit, when you create PRs that touches design, you can add screenshots so it's easier to review :)

Makes sense! Will do here and in the future ;)

Thank you for the help here, too!

@jacobneplokh
Copy link
Member Author

jacobneplokh commented Oct 31, 2020

For the requested reviewers here is a screenshot of the changes:

Screen Shot 2020-10-31 at 11 51 30 AM

@skjnldsv skjnldsv merged commit 862b4a0 into master Nov 1, 2020
@skjnldsv skjnldsv deleted the fix-bold-and-font-size branch November 1, 2020 15:09
@skjnldsv
Copy link
Member

skjnldsv commented Nov 1, 2020

/backport to stable20

Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

Any particular reason behind lowering the font size in the search box? Shouldn't we keep 15px?

@skjnldsv
Copy link
Member

skjnldsv commented Nov 2, 2020

Right, we're now using 15. Let's fix this with #23798

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews design Design, UI, UX, etc. enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reduce font size and remove bold of search empty content notice

5 participants