search

BuddhaNexus / buddhanexus-frontend-next

BuddhaNexus Frontend 2.0
buddhanexus-frontend-next.vercel.app
2 stars 1 forks source link

Text view: part 2 #124

Closed TrebuhD closed 3 months ago

TrebuhD commented 4 months ago
TrebuhD commented 3 months ago

Thank you again for the review @aminahbl . About the points you've mentioned in your main post: Inaccessible color contrast - I have hopefully fixed this in the new ("Reds") color scheme. For the old color scheme, I purposefully didn't touch it, so it should be as accessible as on the old website. In the dark mode, the colors are now inverted, so the contrast should be the same (at least I think so!). I'll test this again on FF.

  1. Thanks for mentioning this, I think I managed to solve it - it was a caching issue, and potentially a bigger one. Hope it's fixed!
  2. We're definitely aiming to support Firefox. For me, the site looks well on Firefox and all the features I've tried work fine. image
  3. I have reverted the text size changes from this PR and adopted yours from the global search PR. I'm happy to go with your recommendations for all text sizes!

I have responded to the rest of the comments and implemented changes - this PR is now ready for re-review! @aminahbl I'll now have a look at your global search PR.

aminahbl commented 3 months ago

Great work @TrebuhD! :clap: Most issues are resolved brilliantly. Per your comment, there is still an issue with colour contrast, but I'm happy for this to be merged and come back to it at a later point with feedback from Orna.

aminahbl commented 3 months ago

@TrebuhD, just following-up on this. I'm currently working on the sidebar update discussed in Slack, and in the process noticed that the text-view colouring only works with default filter settings. Bit of an oversight on my review, sorry! :facepalm:

I think it's important to get all the latest work merged, as I'm already struggling with conflicts am cautiously negotiating between the choice of deferring working on the project, or letting the conflict burden grow. All the same it might be good to at least add a todo...?

TrebuhD commented 3 months ago

hey @aminahbl , thanks for bringing this up! I don't see why the coloring wouldn't work with the other filter settings, is this something I should look at before we merge this one? Otherwise I'm happy to merge now and resolve it ASAP.

aminahbl commented 3 months ago

Yeah, I also can't see why it wouldn't work, but any time I adjust a filter it goes to regular text... maybe something about the data return..? Who knows. In any case, I'd definitely say merge and we can come back to it.