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

Fix#2373 #3473

Merged
merged 6 commits into from Apr 15, 2020
Merged

Fix#2373 #3473

merged 6 commits into from Apr 15, 2020

Conversation

arcturus140
Copy link
Contributor

@arcturus140 arcturus140 commented Feb 16, 2020

Description

This pull request provides additional styling for code elements.

before

light

after

light

Disclaimer:

Due to the many variations in monitors and browsers, color samples may appear different on different monitors.

Issue fixed

This PR automatically closes #2373 .

Type of changes

  • 🔵 Bugfix

@Flexo013 Flexo013 added the awaiting review ❇️ Pull request is awaiting a review. label Feb 16, 2020
@ZeroX-DG
Copy link
Member

I'll test this when I get home

@ZeroX-DG
Copy link
Member

@arcturus140 Looks cool, the CSS works but it doesn't get loaded for users who already have setting....how can we fix this issue?

@ZeroX-DG ZeroX-DG added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Feb 18, 2020
@arcturus140
Copy link
Contributor Author

@ZeroX-DG hm, let me think about it.

If I add it hard coded I am not sure if the user can override it with custom CSS. I'll try this first.

Otherwise maybe add a reset button to override the current custom CSS, like in Boost Note.

@ZeroX-DG
Copy link
Member

Maybe just leave it like that, if old user want to update their custom CSS, we can told them to paste these CSS in themselves? @Flexo013 do you have any idea on this?

@arcturus140
Copy link
Contributor Author

@ZeroX-DG issue 2373 is considered a bug, therefore the user most likely wants to have this change applied and this PR should be a bugfix.

I'll make some changes, maybe even by applying some CSS to default and other to custom CSS.

Not sure if I work on this this weekend. Do you think default look should be more decent or can it stay as is?

@arcturus140
Copy link
Contributor Author

how can I change this into WIP?

@arcturus140 arcturus140 changed the title Fix#2373 [WIP] Fix#2373 Feb 21, 2020
@arcturus140
Copy link
Contributor Author

@ZeroX-DG do you know where settings are stored on linux when using yarn run dev?

I know the packaged install is in .config/ but not the developer instance.

@arcturus140
Copy link
Contributor Author

the CSS styling is now in the code. Additionally, a sample has been added to the custom CSS so the user can simply make the modifications to his liking. 👍

ok, I think that's it, I'm done! 😸

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

Very cool, I think the users can still override your css using !important if they want to. Thank you 👍

@ZeroX-DG ZeroX-DG added approved 👍 Pull request has been approved by sufficient reviewers. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Feb 23, 2020
@ZeroX-DG ZeroX-DG changed the title [WIP] Fix#2373 Fix#2373 Feb 23, 2020
@Rokt33r
Copy link
Member

Rokt33r commented Feb 24, 2020

@arcturus140 Some people might not like this change. What should we do if people don't like this coloring?

@Rokt33r Rokt33r self-requested a review February 24, 2020 07:53
@Rokt33r Rokt33r added awaiting review ❇️ Pull request is awaiting a review. discussion 💬 Issue concerns a discussion. and removed approved 👍 Pull request has been approved by sufficient reviewers. labels Feb 24, 2020
@ZeroX-DG
Copy link
Member

ZeroX-DG commented Feb 24, 2020

@Rokt33r they can simply override it using the custom CSS feature.

Before:
image

After:
image

CSS

[data-theme="default"] p code,
[data-theme="default"] li code
{
  background-color: black;
  border-color: #d9d9d9;
  color: red;
}

@Rokt33r
Copy link
Member

Rokt33r commented Feb 24, 2020

Hmm... I see. But I still want to see more feedback before I merge this.

@arcturus140
Copy link
Contributor Author

@Rokt33r Additionally, an example is provided in the custom CSS text field in preferences so the user knows which elements to apply styling on. One can remove the border, the rounded edges, or add additional styling ...

Of course we can also default to anything we want so feedback is much appreciated 😺

@ZeroX-DG
Copy link
Member

ZeroX-DG commented Mar 21, 2020

@arcturus140 can you resolve the conflict please?

@ZeroX-DG
Copy link
Member

@arcturus140 PING

@Rokt33r
Copy link
Member

Rokt33r commented Apr 10, 2020

@ZeroX-DG @arcturus140 I'll deal with this and merge it in this weekend. So you don't need to fix conflicts.

@Rokt33r Rokt33r added this to the v0.15.3 milestone Apr 11, 2020
@Rokt33r
Copy link
Member

Rokt33r commented Apr 15, 2020

Kazam_screenshot_00000
Kazam_screenshot_00001

The issue is about the style in dark themes. So I'm going to choose more neutral colors.

@Rokt33r Rokt33r merged commit b56e0b9 into BoostIO:master Apr 15, 2020
@Rokt33r Rokt33r added approved 👍 Pull request has been approved by sufficient reviewers. and removed awaiting review ❇️ Pull request is awaiting a review. discussion 💬 Issue concerns a discussion. labels Apr 15, 2020
@Rokt33r Rokt33r removed their request for review April 15, 2020 15:55
@Rokt33r Rokt33r self-assigned this Apr 15, 2020
@arcturus140
Copy link
Contributor Author

@Rokt33r looks great, I like it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved 👍 Pull request has been approved by sufficient reviewers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inline code is hard to see. Better distinction is needed.
4 participants